Closed ehpor closed 7 months ago
Attention: Patch coverage is 90.64748%
with 13 lines
in your changes are missing coverage. Please review.
Project coverage is 82.05%. Comparing base (
9bc1797
) to head (46627de
).
Files | Patch % | Lines |
---|---|---|
hcipy/field/field.py | 91.04% | 12 Missing :warning: |
hcipy/interpolation/linear.py | 50.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@syhaffert Would you mind taking a first quick review pass on this? No rush whatsoever: I'm still working out some kinks on how to test this. But the general structure is there and this is quite an involved transition. All tests pass locally on both new-style and old-style grids.
I am currently reading up on the new way of sub-classing and interoperability with numpy. When I am done with that I might have more comments. But from what I can see now, I think this implementation is looking pretty good. It's pretty logical.
There are now a couple places where you explicitly cast to an array (np.asarray) is that to make it work with non array type objects?
There are now a couple places where you explicitly cast to an array (np.asarray) is that to make it work with non array type objects?
Those are actually all cases where it should've been a numpy array in the first place and not a Field. It's just that a Field can end up in there and you wouldn't notice normally if there is no explicit check for that, like is the case currently.
I added unit tests for basic arithmetic and pickling, that is performed for both old- and new-style fields. While this is not a complete end-to-end test, this unit test checks most things that are used in the rest of the codebase.
There are several improvements that can be made still. A major example is the use of Field for after __array_ufunc__
and __array_function__
. Recasting into a Field afterwards, in case the resulting object is an array, is insufficient. There are several objects that should have a custom Grid attached, which can be inferred from the arguments to the function. This is clearly a major effort that requires a separate PR. As is, however, the current code mimics the logic that exists for old-style fields. Therefore, I'm fine with merging as is.
Another potential improvements is explicit use of np.array([])
in __setstate__()
of NewStyleField
. For non-Numpy objects, pickling will not work. Right now, I don't know how to solve this, so this will have to wait for a future PR. Additionally, this is not needed as the goal of this PR is purely an underlying infrastructure change rather than adding new functionality that is enabled by the underlying infrastructure changes.
@syhaffert Ready for final review.
This PR starts the transition to a new implementation of Fields that is more amenable to backend changes later on. We will refer to the current style of Fields as OldStyleField and the new implementation as NewStyleField. The OldStyleFields are based on subclassing numpy arrays. At the time HCIPy was written, subclassing was the only way to associate a Grid with a Field. Subclassing has several disadvantages, most notably that we fully rely on Numpy to "remember" that the array is in fact a Field. This has been a problem early-on, with the
np.angle()
function, which would not properly call__array_finalize__()
.Recent NEPs have made it more and more possible to avoid subclassing, and instead create a completely new class that behaves just like a numpy array. This approach has been successfully used by Dask, CuPy and others, and is now fully mature. The NewStyleField class uses this implementation, with a
Field.data
attribute that contains the values and aField.grid
attribute that contains the grid as before.Since this is a huge transition, that will affect everyone if done improperly, this will be done with feature flags, which will be turned off for now. This allows everyone to test their code, on an opt-in basis, until we deem that the feature is mature enough to enable by default. Even later, the old-style fields and the corresponding feature flag will be removed.
Open questions: