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

Iterative refinement support for JAX and NumPy forward (spherical) transform implementations #241

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

matt-graham
Copy link
Collaborator

Should resolve #139

Adds support for using iter keyword argument to s2fft.forward when using method = "numpy" and method = "jax", using an iterative refinement approach equivalent to that implemented in healpy to improve accuracy of forward transform (in terms of acting as the inverse of the inverse transform).

Currently this is only added for spherical transform, and is available for all sampling types, even though in practice probably only useful for HEALPix (though iter=1 does seem to give a small gain in accuracy for other sampling types).

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.11%. Comparing base (909e6f1) to head (bb04f00).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   96.07%   96.11%   +0.04%     
==========================================
  Files          31       31              
  Lines        3567     3581      +14     
==========================================
+ Hits         3427     3442      +15     
+ Misses        140      139       -1     

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

@CosmoMatt
Copy link
Collaborator

@matt-graham looks great, one thing that would also be good to add is the same iterations but for the precompute versions of the transforms.

@jasonmcewen
Copy link
Contributor

Good point @CosmoMatt (re precompute). We could either do that in the current PR if quick or else do it in a next one.

@matt-graham
Copy link
Collaborator Author

@CosmoMatt - just to check by precompute versions, do you mean the forward and inverse functions in s2fft/precompute_transforms/spherical.py?

Also, with regards to the precomps argument to the forward and inverse functions in s2fft/transforms/spherical.py - at the moment the current implementation passes whatever value for precomps is passed to the top level forward call to both the underlying forward and inverse function calls. With precomps = None this works fine, but thinking about it I think this will give incorrect results if precomps != None as presumable the precomputed values for forward and inverse transforms differ? Also as we are making multiple calls to the forward and inverse transforms when iter > 0 presumably it is always worth pre-computing in this situation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support HEALPix iterations to improve accuacy of transforms
3 participants