Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New-style Fields #187

Merged
merged 10 commits into from
Mar 5, 2024
Merged

New-style Fields #187

merged 10 commits into from
Mar 5, 2024

Conversation

ehpor
Copy link
Owner

@ehpor ehpor commented Apr 24, 2023

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 a Field.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:

  • How to properly test with feature flags? Currently, tests are only run for the default configuration. Tests should most likely be run for both old- and new-style Fields.

@ehpor ehpor added the enhancement New feature or request label Apr 24, 2023
@ehpor ehpor self-assigned this Apr 24, 2023
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

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 ⚠️
hcipy/interpolation/linear.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   81.90%   82.05%   +0.15%     
==========================================
  Files         101      101              
  Lines        7433     7552     +119     
==========================================
+ Hits         6088     6197     +109     
- Misses       1345     1355      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ehpor
Copy link
Owner Author

ehpor commented Apr 28, 2023

@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.

@syhaffert
Copy link
Collaborator

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?

@ehpor
Copy link
Owner Author

ehpor commented Dec 10, 2023

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.

@ehpor
Copy link
Owner Author

ehpor commented Feb 22, 2024

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.

Copy link
Collaborator

@syhaffert syhaffert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think its ready for merging.

@ehpor ehpor merged commit 7aa6a85 into master Mar 5, 2024
19 checks passed
@ehpor ehpor deleted the feature/new_fields branch March 5, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants