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

Autodiff wrt to trajectory #116

Merged
merged 55 commits into from
Jun 6, 2024
Merged

Autodiff wrt to trajectory #116

merged 55 commits into from
Jun 6, 2024

Conversation

alineyyy
Copy link
Contributor

This resolves #103

@alineyyy alineyyy requested a review from chaithyagr May 24, 2024 13:34
@paquiteau
Copy link
Member

@matthieutrs if you want to have a look ;)

Copy link
Member

@chaithyagr chaithyagr left a comment

Choose a reason for hiding this comment

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

Thank you for great work! Not yet done with review, but a couple of issues to fix

src/mrinufft/operators/autodiff.py Outdated Show resolved Hide resolved
src/mrinufft/operators/autodiff.py Outdated Show resolved Hide resolved
src/mrinufft/operators/autodiff.py Outdated Show resolved Hide resolved
Copy link
Member

@chaithyagr chaithyagr left a comment

Choose a reason for hiding this comment

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

A couple of things to be discussed generally @paquiteau

tests/test_autodiff.py Outdated Show resolved Hide resolved
tests/test_autodiff.py Outdated Show resolved Hide resolved
tests/test_autodiff.py Outdated Show resolved Hide resolved
tests/test_autodiff.py Outdated Show resolved Hide resolved
@chaithyagr
Copy link
Member

@paquiteau I handled the minor details.. locally my tests are passing. Ill add you to officially review it in free time. @alineyyy will handle your review comments. I shall try to see if I can support this in gpuNUFFT in meantime. Great job everyone!

@chaithyagr
Copy link
Member

Ahhh I forgot, @alineyyy you need to do the same updates to finufft interface...

Copy link
Member

@chaithyagr chaithyagr left a comment

Choose a reason for hiding this comment

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

LGTM, Adding @paquiteau for some final checks and then we can merge when green.

@chaithyagr chaithyagr requested a review from paquiteau June 3, 2024 14:58
Copy link
Member

@paquiteau paquiteau left a comment

Choose a reason for hiding this comment

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

This is a major a milestone for MRI-NUFFT, and we are never been so close of merging. Thanks @alineyyy for making this possible !

Some comments/questions to address, and a few suggestion for making things even better.

src/mrinufft/operators/autodiff.py Outdated Show resolved Hide resolved
src/mrinufft/operators/autodiff.py Outdated Show resolved Hide resolved
(x,) = ctx.saved_tensors
return ctx.nufft_op.adj_op(dy), None
(x, traj) = ctx.saved_tensors

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
grad_data = None
grad_traj = None

Comment on lines 60 to 61
else:
grad_traj = None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
else:
grad_traj = None

src/mrinufft/operators/autodiff.py Outdated Show resolved Hide resolved
from ..base import FourierOperatorCPU
from mrinufft._utils import proper_trajectory
from mrinufft._utils import proper_trajectory, get_array_module


def get_fourier_matrix(ktraj, shape, dtype=np.complex64, normalize=False):
"""Get the NDFT Fourier Matrix."""
Copy link
Member

Choose a reason for hiding this comment

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

some documentation as well if possible ?

src/mrinufft/operators/interfaces/nudft_numpy.py Outdated Show resolved Hide resolved
src/mrinufft/operators/interfaces/nudft_numpy.py Outdated Show resolved Hide resolved
tests/case_trajectories.py Outdated Show resolved Hide resolved
tests/operators/test_autodiff.py Show resolved Hide resolved
@paquiteau paquiteau linked an issue Jun 5, 2024 that may be closed by this pull request
Comment on lines 319 to 320
if not AUTOGRAD_AVAILABLE or not self.autograd_available:
raise ValueError("Autograd not available, ensure torch is installed.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not AUTOGRAD_AVAILABLE or not self.autograd_available:
raise ValueError("Autograd not available, ensure torch is installed.")
if not AUTOGRAD_AVAILABLE:
raise ValueError("Autograd not available, ensure torch is installed.")
if not self.autograd_available:
raise ValueError("Backend does not support auto-differentiation.")

self.shape,
self.n_trans,
self.eps,
dtype="complex64" if self.samples.dtype == "float32" else "complex128",
Copy link
Member

Choose a reason for hiding this comment

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

you can use DTYPE_R2C dict for that

Comment on lines 44 to 48
matrix = (
xp.exp(-2j * xp.pi * traj_grid).to(dtype).to(device).clone()
if xp.__name__ == "torch"
else (xp.exp(-2j * xp.pi * traj_grid, dtype=dtype))
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
matrix = (
xp.exp(-2j * xp.pi * traj_grid).to(dtype).to(device).clone()
if xp.__name__ == "torch"
else (xp.exp(-2j * xp.pi * traj_grid, dtype=dtype))
)
matrix = xp.exp(-2j * xp.pi * traj_grid)
if xp.__name__ == "torch":
matrix.to(dtype=dtype, device=device, copy=True)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have to use matrix = matrix.to(...) here, otherwise it will occur an error in the autograd function of pytorch.

Copy link
Member

@paquiteau paquiteau left a comment

Choose a reason for hiding this comment

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

almost there, there are a few comments to address still.

@chaithyagr chaithyagr mentioned this pull request Jun 6, 2024
2 tasks
@chaithyagr
Copy link
Member

If the failing gpunufft test is too strict causing problems, just ensure the gradients are close and maybe just make the test less strict

@paquiteau paquiteau merged commit cfd7470 into master Jun 6, 2024
8 checks passed
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.

Adds support to isign in finufft, cufinufft and gpuNUFFT Add support of autodiff wrt trajectory
3 participants