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

Spiral patch #105

Merged
merged 10 commits into from
May 28, 2024
Merged

Spiral patch #105

merged 10 commits into from
May 28, 2024

Conversation

Daval-G
Copy link
Collaborator

@Daval-G Daval-G commented May 19, 2024

This PR updates or adds the following features, summarized graphically in the picture down below:

  1. Update spiral parameterization from radial to polar: Algebraic spirals (also known as general archimedean spirals) are usually defined in polar coordinate with the radius as a function of the polar angle (polar parameterization), but the opposite can be done too. It was a tricky question that was solved previously by defining the polar angle as a function of the radius (radial parameterization) because the implementation was convenient. However it makes the radius increase by a constant value, which leads to non-senses when referring to the literature, 3D FLORET in particular. Indeed, the goal of using Fermat spirals is to reduce the amount of points close to the center, increase them at the edges to achieve less central redundancy and a better edge coverage while decreasing the gradient strength required. The opposite happens with radial parameterization, and therefore it was changed to polar parameterization. This change mostly improve Fermat-based trajectories, while Archimedes spirals are not affected at all by this change. The power values were reversed to match this polar parameterization. However polar parameterization creates anomalies at the center for spirals with powers superior to 2 such as Fermat, fixed with a new feature explained below.
  2. Remove lithuus, hyperbolic and all spirals with negative powers: Algebraic spirals with negative power values such as Lithuus or Hyperbolic spirals are irrelevant for MRI as their behavior is asymptotically chaotic when approaching the center, without possibility to fix it. The initialize_2D_spiral function now raises an error when negative values are given as arguments.
  3. Add 2D Fibonacci spiral trajectory: Some literature also exists about non-algebraic spirals, and I proposed to reproduce the one from Cline, Harvey E., and Thomas R. Anthony. "Uniform k-space sampling with an interleaved Fibonacci spiral acquisition." In Proceedings of the 7th Annual Meeting of ISMRM, Philadelphia, USA, vol. 1657. 1999. as initialize_2D_fibonacci_spiral. The idea is to produce a strictly uniform density by using Fibonacci circle lattices, split into individual center-out spirals that are slightly eccentric to avoid redundancy. The number of spirals is required to be on the Fibonacci sequence, and therefore additional helper functions are provided in mrinufft.trajectories.maths.
  4. Add function to fix gradient central anomalies: Most spiral-based trajectories from the literature have slew rate anomalies close to the center or poles (algebraic spirals with polar parameterization, Fibonacci spirals, Seiffert spirals, Concentric/Helical shells). A function is added to mrinufft.trajectories.gradients to enforce non-strictly increasing monoticity. This fix is added as an option to initialize_2D_spirals and initialize_2D_fibonacci_spiral, and will be added soon to initialize_3D_seiffert_spiral and initialize_3D_helical_shell in another PR.
  5. Add alias for Galilean spirals: An alias is added for Galilean spirals, another type of algebraic spirals.

This PR is the first one to propose a function to adjust gradients, but more are coming as it is a major element of trajectory design, often present in the literature.

Screenshot from 2024-05-19 15-29-01

@Daval-G Daval-G added the trajectories Issues concerning Non cartesian trajectories label May 19, 2024
@Daval-G Daval-G requested a review from paquiteau May 19, 2024 14:02
@Daval-G Daval-G self-assigned this May 19, 2024
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.

Awesome work ! Nothing much to change, I just made a few comments for the posterity. Good to merge once the CI is happy.

src/mrinufft/trajectories/gradients.py Show resolved Hide resolved
src/mrinufft/trajectories/maths.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/maths.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/trajectory2D.py Outdated Show resolved Hide resolved
@Daval-G Daval-G requested a review from paquiteau May 23, 2024 10:36
@Daval-G
Copy link
Collaborator Author

Daval-G commented May 23, 2024

I had to update the gif examples to make them work again, and I took the occasion to clean them a bit

@paquiteau
Copy link
Member

I think you cleaned up the gif examples a bit too much. the docs fails to build locally. (try python -m docs docs_build to check it)

../examples/example_gif_3D.py unexpectedly failed to execute correctly:

    Traceback (most recent call last):
      File "/volatile/home/pc266769/gits/nsp/code/mri-nufft/examples/example_gif_3D.py", line 203, in <module>
        final_dir = Path(__file__).parent / "docs" / "generated" / "autoexamples" / "images"
    NameError: name '__file__' is not defined

WARNING: g gallery for generated/autoexamples... [ 73%] example_gif_2D.py
../examples/example_gif_2D.py unexpectedly failed to execute correctly:

    Traceback (most recent call last):
      File "/volatile/home/pc266769/gits/nsp/code/mri-nufft/examples/example_gif_2D.py", line 198, in <module>
        final_dir = Path(__file__).parent / "docs" / "generated" / "autoexamples" / "images"
    NameError: name '__file__' is not defined

I must admit that don't know why this happens, since you apparently did not touch the file generation parts of the script.

@paquiteau paquiteau added this to the v0.9.0 milestone May 23, 2024
@Daval-G
Copy link
Collaborator Author

Daval-G commented May 24, 2024

The gifs are not appearing in the current master documentation either, is it expected ?

@Daval-G
Copy link
Collaborator Author

Daval-G commented May 24, 2024

I confirm that this issue is also found in the master branch.

@paquiteau
Copy link
Member

okay, in this case it will be tackle separately I guess (typically in the trajectory gallery PR we are avoiding for now)

@Daval-G
Copy link
Collaborator Author

Daval-G commented May 28, 2024

Good to merge ? @paquiteau

@paquiteau paquiteau merged commit 99c0711 into mind-inria:master May 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trajectories Issues concerning Non cartesian trajectories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants