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

Add ifft helpers, basic tests #94

Closed
wants to merge 5 commits into from
Closed

Conversation

WardBrian
Copy link
Collaborator

Closes #87

Copy link
Collaborator

@eickenberg eickenberg left a comment

Choose a reason for hiding this comment

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

good to go as is, but some questions about naming and defaults came to mind

docs/api.rst Outdated Show resolved Hide resolved
pytorch_finufft/functional.py Show resolved Hide resolved
pytorch_finufft/functional.py Outdated Show resolved Hide resolved
pytorch_finufft/functional.py Show resolved Hide resolved
pytorch_finufft/functional.py Outdated Show resolved Hide resolved
tests/test_inverses.py Show resolved Hide resolved
import pytorch_finufft


def check_t2_ifft_undoes_t1(N: int, dim: int, device: str) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

it strikes me that maybe we need to name t1_ifft to be the one that is actually type 2 ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, happy to discuss naming. We could also be clearer in the docstring which function this inverts, not just which one it wraps

tests/test_inverses.py Show resolved Hide resolved

def check_t1_ifft_undoes_t2(N: int, dim: int, device: str) -> None:
"""
Tests that nuifft_type1 undoes nufft_type2
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar Q: should we name the ifft by what type it actually is or what type it is undoing?

@WardBrian WardBrian marked this pull request as draft October 24, 2023 16:21
@WardBrian WardBrian force-pushed the feature/ifft-helpers branch from 0256e83 to a6cac23 Compare October 25, 2023 13:42
@WardBrian
Copy link
Collaborator Author

I've made the suggested changes up to the point of a possible re-naming.

I did have one question @eickenberg, which is that the finufft docs seem to go out of their way to state that they do not provide inverse transforms. See, e,g: https://finufft.readthedocs.io/en/latest/trouble.html#mathematical-issues-and-advice

The type 1 and type 2 transforms are adjoints but not inverses of each other (unlike in the plain FFT case, where, up to a constant factor , the adjoint is the inverse)

or https://finufft.readthedocs.io/en/latest/overview.html#why-finufft-features-and-comparison-against-other-cpu-nufft-libraries

Note that there are other tasks (eg, transforms on spheres, inverse NUFFTs) provided by other libraries, such as NFFT3, that FINUFFT does not provide.

Are we using 'inverse' in a different sense than they mean?

@WardBrian
Copy link
Collaborator Author

Yep, treating the adjoint like the inverse would be bad. Closing this and the related issue

@WardBrian WardBrian closed this Oct 25, 2023
@WardBrian WardBrian deleted the feature/ifft-helpers branch October 25, 2023 20:31
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.

ENH ifft functions or ifft mode in our current functions?
2 participants