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.

Sign up for GitHub

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

Jump to bottom

Make Trajectory and Idx 5D #660

Merged
merged 237 commits into from
Mar 5, 2025
Merged

Make Trajectory and Idx 5D #660

merged 237 commits into from
Mar 5, 2025

Conversation

fzimmermann89
Copy link
Member

@fzimmermann89 fzimmermann89 commented Feb 28, 2025 •
edited
Loading

Would be great if somebody could lend another pair of eyes if I got all tests.

fzimmermann89 and others added 30 commits November 26, 2024 15:46
@fzimmermann89
Update
00ecea0 
[ghstack-poisoned]
@fzimmermann89
Update
15964ab 
[ghstack-poisoned]
@fzimmermann89
Update
9bf8253 
[ghstack-poisoned]
@fzimmermann89
Update
3e66095 
[ghstack-poisoned]
@fzimmermann89
Update
7dd0b2e 
[ghstack-poisoned]
@fzimmermann89
Update
7e5c310 
[ghstack-poisoned]
@fzimmermann89
Update
49c813b 
[ghstack-poisoned]
@fzimmermann89
Update
523fb10 
[ghstack-poisoned]
@fzimmermann89
Update
6e72faa 
[ghstack-poisoned]
@fzimmermann89
Update
f3a1baf 
[ghstack-poisoned]
@fzimmermann89
Update
7d7cb72 
[ghstack-poisoned]
@fzimmermann89
Update
6cab1a4 
[ghstack-poisoned]
@fzimmermann89
Update
3057f5c 
[ghstack-poisoned]
@fzimmermann89
Update
411127c 
[ghstack-poisoned]
@fzimmermann89
Update
7e2e5e8 
[ghstack-poisoned]
@fzimmermann89
Update
f819f91 
[ghstack-poisoned]
@fzimmermann89
Update
3cc4e09 
[ghstack-poisoned]
@fzimmermann89 @ckolbPTB
Apply suggestions from code review
8611f77 
Co-authored-by: Christoph Kolbitsch <[email protected]>
@fzimmermann89
Update
7ff70bc 
[ghstack-poisoned]
@fzimmermann89
Update
1305a52 
[ghstack-poisoned]
@fzimmermann89
Update
ace6ec7 
[ghstack-poisoned]
@fzimmermann89
Update
d8cb4ce 
[ghstack-poisoned]
@fzimmermann89
Update
664e312 
[ghstack-poisoned]
@fzimmermann89
Update
c01d972 
[ghstack-poisoned]
@fzimmermann89
Update
9e33f06 
[ghstack-poisoned]
@fzimmermann89
Update
ff6adbd 
[ghstack-poisoned]
@fzimmermann89
Update
ac836a7 
[ghstack-poisoned]
@fzimmermann89
Update
cc03ed0 
[ghstack-poisoned]
@fzimmermann89
Update
5122717 
[ghstack-poisoned]
@fzimmermann89
Update
2e3d5ae 
[ghstack-poisoned]
@koflera
Copy link
Collaborator

koflera commented Mar 4, 2025 •
edited
Loading

In

mrpro/tests/data/test_trajectory.py

Line 153 in 20b1d10

def test_ktype_along_kzyx(im_shape, k_shape, nkx, nky, nkz, type_kx, type_ky, type_kz, type_k0, type_k1, type_k2):

im_shape, k_shape are not used. same for

mrpro/tests/data/test_trajectory.py

Line 184 in 20b1d10

def test_ktype_along_k210(im_shape, k_shape, nkx, nky, nkz, type_kx, type_ky, type_kz, type_k0, type_k1, type_k2):

schote pushed a commit that referenced this pull request Mar 4, 2025
@fzimmermann89
Make Trajectory and Idx 5D
56e8b47 
ghstack-source-id: 31a55d2ba75646e42c9896d5d3dc2130c4c80d32
ghstack-comment-id: 2691622453
Pull Request resolved: #660
@koflera
Copy link
Collaborator

koflera commented Mar 5, 2025

k_shape not used in

mrpro/tests/operators/test_non_uniform_fast_fourier_op.py

Line 15 in 20b1d10

def create_data(im_shape, k_shape, nkx, nky, nkz, type_kx, type_ky, type_kz):

@fzimmermann89
Copy link
Member Author

The CommonMrTrajectories decorator adds the arguments.
So if any function needs the image or k shape (which some do) we need to have these parameters in all functions.

We could if they are unused, add an underscore to the name. But is it worth it?

@fzimmermann89
Copy link
Member Author

k_shape not used in

mrpro/tests/operators/test_non_uniform_fast_fourier_op.py

Line 15 in 20b1d10

def create_data(im_shape, k_shape, nkx, nky, nkz, type_kx, type_ky, type_kz):

But here it should be removed, yes

@koflera
Copy link
Collaborator

koflera commented Mar 5, 2025

missing coil dimension in the following?

mrpro/src/mrpro/data/traj_calculators/KTrajectorySunflowerGoldenRpe.py

Line 85 in 20b1d10

angles = repeat((k2_idx * self.angle) % torch.pi, '... k2 k1 -> ... k2 k1 k0', k0=1)

@koflera
Copy link
Collaborator

koflera commented Mar 5, 2025

replace 1 by coils=1 in ?

mrpro/src/mrpro/operators/CartesianSamplingOp.py

Line 90 in 20b1d10

inside_encoding_matrix = rearrange(inside_encoding_matrix, '... 1 kz ky kx -> ... 1 (kz ky kx)')

koflera
koflera requested changes Mar 5, 2025
View reviewed changes
Copy link
Collaborator

@koflera koflera left a comment

Choose a reason for hiding this comment

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

some very small comments, I was not able to find anything obviously wrong

fzimmermann89 added a commit that referenced this pull request Mar 5, 2025
@fzimmermann89
Make Trajectory and Idx 5D
c37af51 
ghstack-source-id: 06161873be1dc924c5c039ffe5a4b2e147ac14a7
ghstack-comment-id: 2691622453
Pull Request resolved: #660
@fzimmermann89
Update
cb282e5 
[ghstack-poisoned]
@fzimmermann89
Update
3fdb83f 
[ghstack-poisoned]
@fzimmermann89
Update
994c9ad 
[ghstack-poisoned]
@fzimmermann89
Update
bf2160f 
[ghstack-poisoned]
@fzimmermann89
Update
fee5000 
[ghstack-poisoned]
@fzimmermann89
Update
cca22e6 
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Mar 5, 2025
@fzimmermann89
Make Trajectory and Idx 5D
8f5acc9 
ghstack-source-id: 8486446026b32b61d71a127abccfd17f942655b3
ghstack-comment-id: 2691622453
Pull Request resolved: #660
@fzimmermann89
Update
33b35f4 
[ghstack-poisoned]
@fzimmermann89
Update
34e7c89 
[ghstack-poisoned]
@fzimmermann89 fzimmermann89 requested a review from koflera March 5, 2025 16:28
Base automatically changed from gh/fzimmermann89/37/head to main March 5, 2025 16:29
@fzimmermann89 fzimmermann89 mentioned this pull request Mar 5, 2025
Merged
@koflera
Copy link
Collaborator

koflera commented Mar 5, 2025

is a 1 missing in the following?

mrpro/src/mrpro/data/DcfData.py

Line 66 in a639f93

dcf = torch.ones(*traj.broadcasted_shape[-3:], 1, 1, 1, device=traj.kx.device)

koflera
koflera approved these changes Mar 5, 2025
View reviewed changes
Copy link
Collaborator

@koflera koflera left a comment

Choose a reason for hiding this comment

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

looks good to me

@fzimmermann89
Copy link
Member Author

is a 1 missing in the following?

mrpro/src/mrpro/data/DcfData.py

Line 66 in a639f93

dcf = torch.ones(*traj.broadcasted_shape[-3:], 1, 1, 1, device=traj.kx.device)

I think it's correct:
Same shape as trajectory, but last three dimensions replaced by 1.

Trajectory got another dimension, so did DCF. So it still matches?

@fzimmermann89 fzimmermann89 merged commit 08a60e9 into main Mar 5, 2025
21 checks passed
@fzimmermann89 fzimmermann89 deleted the gh/fzimmermann89/46/head branch March 5, 2025 19:13
@fzimmermann89 fzimmermann89 mentioned this pull request Mar 6, 2025
Merged
fzimmermann89 added a commit that referenced this pull request Mar 14, 2025
@fzimmermann89
Squashed commit of the following:
64af75a 
commit 7a6ad39
Author: Felix Zimmermann <[email protected]>
Date:   Fri Mar 14 01:40:07 2025 +0100

    py310

commit cf58e03
Author: Felix Zimmermann <[email protected]>
Date:   Fri Mar 14 01:34:30 2025 +0100

    py310

commit 9caa056
Author: Felix Zimmermann <[email protected]>
Date:   Fri Mar 14 01:31:23 2025 +0100

    default

commit 85b0fd9
Author: Felix Zimmermann <[email protected]>
Date:   Fri Mar 14 01:28:53 2025 +0100

    fix

commit 6785ccf
Author: Felix Zimmermann <[email protected]>
Date:   Fri Mar 14 01:10:50 2025 +0100

    add avg op

commit 704fecd
Author: Stefan Martin <[email protected]>
Date:   Tue Mar 11 11:31:43 2025 +0100

    Fix examples in readme (#713)

commit 77290ef
Author: bernharb <[email protected]>
Date:   Mon Mar 10 12:13:56 2025 +0100

    Fix naming of RearrangeOp tests (#711)

commit 2e6e0d0
Author: bernharb <[email protected]>
Date:   Mon Mar 10 11:19:56 2025 +0100

    Add tolerance to rotation mean test (#710)

commit d87ab35
Author: Leonid Lunin <[email protected]>
Date:   Fri Mar 7 15:03:53 2025 +0100

    Add python 3.13 support (#704)

commit d286092
Author: Felix F Zimmermann <[email protected]>
Date:   Fri Mar 7 14:27:29 2025 +0100

    Fix batched complex grid sampling (#695)

commit d52987d
Author: Christoph Kolbitsch <[email protected]>
Date:   Fri Mar 7 14:22:08 2025 +0100

    Update readme on fork and pull request (#653)

commit 133a11e
Author: Stefan Martin <[email protected]>
Date:   Fri Mar 7 11:06:05 2025 +0100

    Change iteration argument name in optimizers (#687)

    Co-authored-by: Felix F Zimmermann <[email protected]>

commit 2988477
Author: Felix F Zimmermann <[email protected]>
Date:   Thu Mar 6 20:26:00 2025 +0100

    Add Jacobian of Operator (#452)

commit 6a9eb16
Author: rkcatarina <[email protected]>
Date:   Thu Mar 6 20:09:11 2025 +0100

    Update zenodo citation (#698)

commit 3aa5aeb
Author: Felix F Zimmermann <[email protected]>
Date:   Thu Mar 6 20:04:43 2025 +0100

    Reduce RAM usage in tests (#703)

commit 870967c
Author: Mara Guastini <[email protected]>
Date:   Thu Mar 6 16:32:33 2025 +0100

    Add stopping criterion to optimizer callback (#699)

commit 10aa16c
Author: Felix F Zimmermann <[email protected]>
Date:   Thu Mar 6 16:17:03 2025 +0100

    Indexing helper (#672)

commit 15060bf
Author: Christoph Kolbitsch <[email protected]>
Date:   Thu Mar 6 10:56:11 2025 +0100

    Add gradient and autodiff checks for linear operators (#536)

    Co-authored-by: Felix F Zimmermann <[email protected]>

commit 6d4b221
Author: Felix F Zimmermann <[email protected]>
Date:   Thu Mar 6 10:50:30 2025 +0100

    Release v0.250306 (#697)

commit ad7b6e0
Author: rkcatarina <[email protected]>
Date:   Thu Mar 6 01:12:25 2025 +0100

    Rename ti to saturation_time in saturation recovery model (#689)

commit 3839a07
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 20:23:51 2025 +0100

    Refactor signal model tests (#671)

commit d4bd171
Author: Stefan Martin <[email protected]>
Date:   Wed Mar 5 20:18:48 2025 +0100

    Add __repr__ to Fourier operators (#676)

    Co-authored-by: Felix F Zimmermann <[email protected]>

commit 08a60e9
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 20:13:45 2025 +0100

    Make Trajectory and Idx 5D (#660)

    Makes all of our dataclasses at least 5D.
    So acq_idx get a (singleton) k0 dimension, trajectory a singleton coil dimension etc.

commit a7a5e2f
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 17:34:49 2025 +0100

    Use gram in operator_norm (#682)

commit c301e1b
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 17:29:40 2025 +0100

    Add more information to IHeader (#591)

commit 65b965c
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 17:23:11 2025 +0100

    Rename remove_repeat to reduce_repeat and add tests (#679)

commit 8e79e74
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 17:17:24 2025 +0100

    Refactor AcqInfo and Trajectory Calculation (#560)

commit 14e4058
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 15:32:04 2025 +0100

    Fix torchvision in docker images (#693)

commit 2bd6e29
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 15:19:38 2025 +0100

    Add unsqeeze_at (#656)

commit 274e203
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 15:03:29 2025 +0100

    Make Rotation einops compatible (#655)

commit b9840b8
Author: Felix F Zimmermann <[email protected]>
Date:   Wed Mar 5 14:32:33 2025 +0100

    Add Brainweb phantom (#630)

commit 7fe0014
Author: Mara Guastini <[email protected]>
Date:   Wed Mar 5 13:52:06 2025 +0100

    Add pgd (fista) algorithm (#293)

    Co-authored-by: Felix F Zimmermann <[email protected]>
    Co-authored-by: Christoph Kolbitsch <[email protected]>
    Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
    Co-authored-by: koflera <[email protected]>

commit c80ad02
Author: Andreas Kofler <[email protected]>
Date:   Wed Mar 5 13:20:57 2025 +0100

    Bump pydicom dependency (#685)

commit a14fde8
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Wed Mar 5 10:56:54 2025 +0100

    [pre-commit] pre-commit autoupdate (#681)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koflera koflera koflera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fzimmermann89 @koflera