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

Trajectory display overhaul #37

Merged
merged 13 commits into from
Oct 7, 2023

Conversation

Daval-G
Copy link
Collaborator

@Daval-G Daval-G commented Oct 2, 2023

This PR consists of the following features:

  • New display functions to show 2D and 3D gradients with several options.
  • Updates on the trajectory display functions to properly account for acquisition parameters and hardware constraints.
  • Fixes on wrong units in comments and documentation.
  • A new set of util functions to manipulate and convert trajectory/gradients/slew rates and check gradient constraints, as previous functions were kind of twisted to serve multiple purposes at once, notably for IO.

Currently, the documentation still needs to be written and examples updated as they are the first ones impacted by the display changes. Here is a simple rendering:

image

@philouc
Copy link
Collaborator

philouc commented Oct 2, 2023

We should probably set up a discussion in the coming weeks to see notably how to interface your nice contributions to mri-nufft with the brand new fMRI simulator developed by @paquiteau

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.

Seems very promising, the gradient plot are exactly what I had in mind. There is surely some docs effort to make but this is really good.

src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/io.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/io.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/utils.py Outdated Show resolved Hide resolved
@Daval-G Daval-G requested a review from paquiteau October 5, 2023 16:57
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.

It keeps getting better !
The displayConfig can be simplified though, see my suggestions

src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
src/mrinufft/trajectories/display.py Outdated Show resolved Hide resolved
@Daval-G Daval-G requested a review from paquiteau October 7, 2023 10:05
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.

LGTM !

@Daval-G Daval-G merged commit b3bb069 into mind-inria:master Oct 7, 2023
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.

3 participants