Remove the (NumPy-) backend-specific hierarchy of weighting classes #1686
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The weightings class, as it is currently, aims at equipping a space with a given geometry. For instance, by providing a |
…sses. The previous version made hard-coded decisions of low-level operations. These really are specific to NumPy and the various computational packages. This allowed getting fairly good performance on CPUs. In the future, NumPy will be mostly a fallback; performance-critical applications should use GPU-enabled storage backends instead. This means it is less critical to optimize CPU performance for NumPy, but more important to keep things general. This commit proposes using only NumPy functions that are in the Python Array API for the weighting classes, meaning they can easily be generalised to e.g. PyTorch.
…ckend-agnostic module. Currently, NumPy is the default storage backend anyway and can therefore be used as the base-class implementation. Looking forward, this will be generalized using the Python array API to also support different storage backends with the same class.
…hting` class. Up until now, the NumPy-specific weighting classes allowed passing an element of ODL spaces directly as the weighting. This feature was not really documented, but checked in the tests. Since the weighting classes are more low-level than the classes of spaces, they should not be used like this. Allowing it (which the NumPy version does awkwardly with an instance check on the specific class) is rather a matter of protecting the user from shooting themselves in the foot than a legitimate use case. A better safeguard is to ensure the weightings are an array that is compatible with the specified `impl`.
…spaces. Currently, these classes use NumPy anyway, so no functionality is lost. In the future, they will allow any Array-API arrays that match the `impl` argument, which in the case of `impl='numpy'` will again simply be `np.ndarray`s.
The NumPy-specific ones are about to be removed, in favour of directly offering that functionality for any backend in the base weighting classes via the Python Array API.
These are in the future unnecessary, because the Python Array API will allow using the generic weighting classes for any suitable implementation. They are also currently unnecessary, because NumPy as the only supported backend can be handled with only the base hierarchy without duplicating it into the NumPy-tensor module.
leftaroundabout
force-pushed
the
refactor/weightings/no-backend-specific-subclasses
branch
from
df1b5ff to
5bc657a
Compare
…(backend-agnostic) class.
leftaroundabout
added a commit
to leftaroundabout/odl
that referenced
this pull request
Jun 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The weighting classes (i.e. the classes that allow tensor spaces to be imbued with different norms / inner products from the standard Euclidean ones) have until been duplicate, with each of the
ArrayWeighting,ConstWeighting, and custom weightings appearing both in the baseweightingmodule, and also in an almost identical, but NumPy-specific version in thenpy_tensorsmodule.This duplication made sense in the view that non-NumPy backends might need to implement the functionality in a completely different manner. This newer happened though, with NumPy still being the de-facto single backend.
ODL-1.0 meanwhile shall support different backends without the need for such special casing: through the Python Array API. @Emvlt therefore suggested bundling all of the functionality directly in the
weightingsmodule, without backend-specific versions of the weighting classes. #1683 already contains an implementation of that, which diverges however quite drastically from the existing hierarchy and would pose difficulties when supporting more advanced Hilbert space structures.This PR removes the NumPy-specific classes, while leaving the hierarchy in the
weightingmodule as is, and therefore allows any mathematical functionality supported by the old ODL but also easy generalization using the Python Array API.The only functionality lost here is that the
ArrayWeightingclass can not be initialized with aNumPyTensorobject anymore. As this was never documented anyway, is arguably missing the point of that class, and the behaviour can easily be achieved by using.data, I would not consider this problematic, although I added a TODO concerning that once the Array API is used a better sanity check should be added.