Add prephasors, rewinders etc. #276

chaithyagr · 2025-06-19T08:58:21Z

This is just the same PR #269 . Adding back @Daval-G and @j-obriot for review.

paquiteau

Here are some general comments, I don't understand it fully, but I do see that is great stuff nonetheless !

src/mrinufft/trajectories/tools.py

Co-authored-by: Pierre-Antoine Comby <[email protected]>

src/mrinufft/trajectories/tools.py

… add_prephasors

chaithyagr · 2025-07-28T11:34:23Z

I think finallllyyyy, we have a good PR :) Sorry for the mess, i took most of all your comments in, but I think we need fresh round of reviews given that I did major changes to the codes.

paquiteau

That was a big bite to chew, but nicely cooked !
I have some nitpicks suggestions and questions (That could be answered as comments in code for posterity)

paquiteau · 2025-08-19T07:41:55Z

src/mrinufft/io/nsp.py

                f"Maximum gradient amplitude: {maxG:.3f} > {gmax:.3f}"
                f"Maximum slew rate: {maxS:.3f} > {smax:.3f}"
            )
+        if pregrad != "prephase":


It would be safer without the prephase check (we always want to raise warning if the slew-rate is not hardware compatible)

naahh, i disagree.. As @Daval-G mentioned, the sequence can handle pre-phasors right. This warning shouyld come up only in the case of UTE trajectories.. I think I am handeling that in #273

src/mrinufft/trajectories/tools.py

paquiteau · 2025-08-19T08:17:15Z

src/mrinufft/trajectories/tools.py

+    if ceil:
+        n_ramp_down = np.ceil(n_ramp_down).astype(int)
+        n_ramp_up = np.ceil(n_ramp_up).astype(int)
+    return n_ramp_down + buffer, n_ramp_up + buffer


why the buffer ? Also in the rest of the code it is always equal to 1, consider just inlining it ? (with a comment to say why this buffer is required)

Buffer is just a way to allow for function to add buffers to give some extra rastre times so that the calculation wont fail in case we didnt fully converge or issues due to np.ceil on ramp length (discritization)

src/mrinufft/trajectories/tools.py

tests/test_io.py

Co-authored-by: Pierre-Antoine Comby <[email protected]>

paquiteau

LGTM !
I will see how we can rebase #277 and #283 on top of it properly

Daval-G

I trust @paquiteau's approval and @chaithyagr on recent changes, I will take too much time to review otherwise

* Added * Fix * Remove bymistake add * Fix * Fixed lint * Lint * Added refbackend * Fix NDFT * feat: use finufft as ref backend. * feat(tests): move ndft vs nufft tests to own file. * Add support for pipe * \!docs_build try to run cufinufft tests * \!docs_build fix style * Added next235 for stability * Fix lint * Fix CUPY * WIP * Updates * fix back learn examples * move tto flatiron * fix black * Move to test on GPU * Update pyproject toml and use it in test-ci, to prevent duplication of dependencies and actually test them! * Make CI build shorter * Test run to run * \!docs_build Added * adding density normalization * Start support for finufft spread and interpolate (pipe) * Add support for density comp * Add support for normlize back * Added a bunch of extra codes * PEP fixes * Update siemens.py * Added fixes * add [docs] * Fixes and updates on the locatuions * Update the codes to be in sync with cufinufft / finufft 2.4.0 * Merged to master * [style] * Fix toml ffile [docs] * Fix toml ffile [docs] * Update testbatch stuff * Update the tests * Fixed tests againnnn * All set * Add finufft * Updates * Fixes * More fixes * make tests less strict again * remove bymistake * Remove strictness, everything works, hopefully [docs] * [docs] setup cufinufft also * [docs] fix style * [docs] more updates, fixes * pywavelets * [docs] fixes further * [docs] try again * [docs] final comments * [docs] method class use * WIP, updated codes to get itw orking * trying to vectorize * WIP * WIP * added direct change * More updates * Added timing and gradients * Update gradspec and codes * Update the error for slew * Added tests * Setup PEP, ready for review * Minor update on spec * WIP * Fixes for tests * fix docs * Update src/mrinufft/io/nsp.py Co-authored-by: Pierre-Antoine Comby <[email protected]> * remove unused stuff * Final fixes. [docs] * Haandleing comments * Update src/mrinufft/trajectories/tools.py Co-authored-by: Guillaume Daval-Frérot <[email protected]> * [docs] update the docnames * update TE_pos * style fixes * style fixes * Fix for ramps * Update src/mrinufft/trajectories/tools.py Co-authored-by: Guillaume Daval-Frérot <[email protected]> * Apply suggestions from code review Co-authored-by: Guillaume Daval-Frérot <[email protected]> * Update src/mrinufft/io/nsp.py Co-authored-by: Guillaume Daval-Frérot <[email protected]> * Update src/mrinufft/io/nsp.py Co-authored-by: Guillaume Daval-Frérot <[email protected]> * Update src/mrinufft/io/nsp.py Co-authored-by: Guillaume Daval-Frérot <[email protected]> * Update src/mrinufft/io/nsp.py Co-authored-by: Guillaume Daval-Frérot <[email protected]> * Handle comments * rename parameters and fix ordering * Handle comments * Update codes * More updates * Fixed some bugs * Update the tools * style fixes and [docs] * More bugs! Fixes * [WIP] * Updateds WIP * wip * WIP: moved to solvers * Fixed everything, ready for review * Update src/mrinufft/trajectories/tools.py Co-authored-by: Pierre-Antoine Comby <[email protected]> * style fixes * Update ruff * Added joblib * style changes [docs] * Apply suggestions from code review Co-authored-by: Pierre-Antoine Comby <[email protected]> * Renaming * Fix more comments * Undo changes * fix undo * refactor: split test in two --------- Co-authored-by: chaithyagr <[email protected]> Co-authored-by: Pierre-antoine Comby <[email protected]> Co-authored-by: Asma TANABENE <[email protected]> Co-authored-by: Guillaume Daval-Frérot <[email protected]> Co-authored-by: Pierre-Antoine Comby <[email protected]>

chaithyagr and others added 30 commits April 26, 2024 14:45

Added

8ca012c

Merge branch 'mind-inria:master' into master

3a980c2

Fix

093edfb

Merge branch 'master' of github.com:chaithyagr/mri-nufft

f8a6a4a

Remove bymistake add

74c1ecd

Fix

0250aa8

Fixed lint

060a8bd

Lint

aecb844

Added refbackend

3130bc1

Fix NDFT

bc014b8

feat: use finufft as ref backend.

0cc73c4

feat(tests): move ndft vs nufft tests to own file.

21e090f

Merge branch 'master' of github.com:mind-inria/mri-nufft

6869a4a

Merge branch 'master' of github.com:mind-inria/mri-nufft

f8364d4

Merge branch 'master' of github.com:mind-inria/mri-nufft

23a63da

Merge branch 'master' of github.com:mind-inria/mri-nufft

3709e74

Merge branch 'master' of github.com:chaithyagr/mri-nufft

8d6b9b4

Merge branch 'master' of github.com:mind-inria/mri-nufft

398fb28

git Merge branch 'master' of github.com:mind-inria/mri-nufft

bcf7ce3

Add support for pipe

3da762f

\!docs_build try to run cufinufft tests

d644cf6

\!docs_build fix style

0dca8f6

Added next235 for stability

643e1e9

Fix lint

af6bbfa

Fix CUPY

02c834f

Merge branch 'master' into cufinufft

d50f427

WIP

8cfd427

merge

ab6eaa4

Updates

3c3f1c8

fix back learn examples

bb28eb9

paquiteau requested changes Jul 11, 2025

View reviewed changes

chaithyagr and others added 5 commits July 11, 2025 22:00

[WIP]

7cf0890

Updateds WIP

4cea1ac

wip

9db59e3

WIP: moved to solvers

13f3c13

Fixed everything, ready for review

c182941

chaithyagr requested a review from paquiteau July 25, 2025 12:40

chaithyagr and others added 2 commits July 25, 2025 14:42

Update src/mrinufft/trajectories/tools.py

c18283a

Co-authored-by: Pierre-Antoine Comby <[email protected]>

style fixes

7a447c5

chaithyagr commented Jul 25, 2025

View reviewed changes

src/mrinufft/trajectories/tools.py Show resolved Hide resolved

chaithyagr added 4 commits July 25, 2025 14:49

Merge branch 'add_prephasors' of github.com:mind-inria/mri-nufft into…

8f2fffd

… add_prephasors

Update ruff

12ce016

Added joblib

935b52b

style changes [docs]

1429f27

paquiteau requested changes Aug 19, 2025

View reviewed changes

chaithyagr and others added 6 commits August 19, 2025 10:50

Apply suggestions from code review

386097a

Co-authored-by: Pierre-Antoine Comby <[email protected]>

Renaming

5b9e621

Fix more comments

3450ede

Undo changes

d131f47

fix undo

b9b307f

refactor: split test in two

8e54c61

paquiteau force-pushed the add_prephasors branch from 6f4e1bc to 8e54c61 Compare August 19, 2025 14:23

paquiteau approved these changes Aug 19, 2025

View reviewed changes

Daval-G approved these changes Aug 20, 2025

View reviewed changes

chaithyagr merged commit c7fb11b into master Aug 20, 2025
13 checks passed

paquiteau mentioned this pull request Aug 22, 2025

Faster prephasors with linear programming #286

Open

paquiteau deleted the add_prephasors branch September 24, 2025 16:54

paquiteau added this to the v1.4 milestone Oct 9, 2025

Add prephasors, rewinders etc. #276

Add prephasors, rewinders etc. #276

Uh oh!

Conversation

chaithyagr commented Jun 19, 2025

Uh oh!

paquiteau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chaithyagr commented Jul 28, 2025

Uh oh!

paquiteau left a comment

Choose a reason for hiding this comment

Uh oh!

paquiteau Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

chaithyagr Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paquiteau Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

chaithyagr Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paquiteau left a comment

Choose a reason for hiding this comment

Uh oh!

Daval-G left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants