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

Implement Gram Shortcut for FourierOp #503

Merged
merged 8 commits into from
Nov 14, 2024
Merged

Implement Gram Shortcut for FourierOp #503

merged 8 commits into from
Nov 14, 2024

Conversation

fzimmermann89
Copy link
Member

@fzimmermann89 fzimmermann89 commented Nov 8, 2024

Add Gram Operator to the FourierOp and CartesianSamplingOp

FourierOp:
written as convolution with kernel, making use of toeplitz structure of nufft. kernel is calculated with 2^n_dim adjoint nuffs, instead of one nufft with twice the size in all dimensions, saving on memory. same approach as done by tensorflow MRI, but with a loop over he combinations instead of recursion. The torchkbnufft adjoint nufft can at some point be replaced by a different nufft operation. Currently, the code prodces the same results as https://torchkbnufft.readthedocs.io/en/stable/_modules/torchkbnufft/_nufft/toep.html#calc_toeplitz_kernel
If we slit nufft op out of FourierOp, we can also move the nufft-related stuff from init and forward 1:1 to new nufft op.

CartesianSamplingOp
does forward and adjoint pass on an all-ones mask. this could be optimized to instead just set ones in a all-zero mask. but the current form works and is simple.
If we add CartesianSamplingOp to FourierOp, we would also have to include it in FourierOp.gram on the cartesian dimensions.

Tests compare against Operator.H@Operator as ground-truth.

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms/csm
   inati.py24196%44
   walsh.py16194%34
src/mrpro/algorithms/dcf
   dcf_voronoi.py53492%15, 48–49, 76
src/mrpro/algorithms/optimizers
   adam.py20195%69
src/mrpro/algorithms/reconstruction
   DirectReconstruction.py281643%51–71, 85
   IterativeSENSEReconstruction.py13192%76
   Reconstruction.py502256%42, 54–56, 80–87, 104–113
   RegularizedIterativeSENSEReconstruction.py411759%96–100, 114–139
src/mrpro/data
   AcqInfo.py128398%26, 169, 207
   CsmData.py29390%15, 82–84
   DcfData.py45882%18, 66, 78–83
   IData.py67987%119, 125, 129, 159–167
   IHeader.py75791%75, 109, 127–131
   KHeader.py1531789%25, 119–123, 150, 199, 210, 217–218, 221, 228, 260–271
   KNoise.py311552%39–52, 56–61
   KTrajectory.py69593%178–182
   MoveDataMixin.py1371887%15, 113, 129, 143–145, 207, 305–307, 320, 399, 419–420, 422, 437–438, 440
   QData.py39782%42, 65–73
   Rotation.py6743595%100, 198, 335, 433, 477, 495, 581, 583, 592, 626, 628, 691, 768, 773, 776, 791, 808, 813, 889, 1077, 1082, 1085, 1109, 1113, 1240, 1242, 1250–1251, 1315, 1397, 1690, 1846, 1881, 1885, 1996
   SpatialDimension.py2302191%33, 103, 128, 135, 141, 261–263, 276–278, 312, 330, 343, 356, 369, 382, 391–392, 407, 416
   acq_filters.py12192%47
src/mrpro/data/_kdata
   KData.py1341887%109–110, 125, 132, 142, 150, 204–205, 243, 248–249, 268–279
   KDataRemoveOsMixin.py29293%44, 46
   KDataSelectMixin.py19289%48, 63
   KDataSplitMixin.py48394%53, 84, 93
src/mrpro/data/traj_calculators
   KTrajectoryCalculator.py25292%23, 45
   KTrajectoryIsmrmrd.py13285%41, 50
   KTrajectoryPulseq.py29197%54
src/mrpro/operators
   CartesianSamplingOp.py89397%118, 157, 280
   ConstraintsOp.py60297%46, 48
   EndomorphOperator.py65297%228, 234
   FiniteDifferenceOp.py27293%40, 105
   FourierOp.py157398%263, 381, 386
   Functional.py71593%20–22, 117, 119
   GridSamplingOp.py136993%72–73, 82–83, 90–91, 94, 96, 98
   LinearOperator.py1711293%55, 91, 190, 220, 261, 270, 278, 287, 295, 320, 418, 423
   LinearOperatorMatrix.py1581690%82, 119, 152, 161, 166, 175–178, 191–194, 203, 215, 304, 331, 359
   MultiIdentityOp.py13285%43, 48
   Operator.py78297%25, 74
   ProximableFunctionalSeparableSum.py39392%50, 103, 110
   SliceProjectionOp.py173895%44, 61, 63, 69, 206, 227, 260, 300
   WaveletOp.py120596%152, 170, 205, 210, 233
   ZeroPadOp.py16194%30
src/mrpro/utils
   filters.py62297%44, 49
   slice_profiles.py46687%20, 36, 113–116, 149
   sliding_window.py34197%34
   split_idx.py10280%43, 47
   summarize_tensorvalues.py11918%20–29
   typing.py181139%8–23
   zero_pad_or_crop.py31681%26, 30, 54, 57, 60, 63
TOTAL486535493% 

Tests Skipped Failures Errors Time
2009 0 💤 0 ❌ 0 🔥 2m 15s ⏱️

Copy link
Contributor

github-actions bot commented Nov 8, 2024

📚 Documentation

📁 Download as zip
🔍 View online

@fzimmermann89 fzimmermann89 force-pushed the gram branch 2 times, most recently from 139cc6e to d57aa80 Compare November 9, 2024 13:47
@fzimmermann89 fzimmermann89 marked this pull request as ready for review November 9, 2024 13:48
@fzimmermann89
Copy link
Member Author

fzimmermann89 commented Nov 11, 2024

This will need an adjustment once the CartesianSamplingOp is added to FourierOp. ✔️

Should we keep the helper function that creates the convolution kernel next to the Operators or should we move it into algorithms somewhere?

It is IMHO not really an algorithm, just a tiny bit smarter way to do a Nufft with doubled images size in each dimension..

@fzimmermann89 fzimmermann89 added the pre-commit.ci autofix run autofix in this PR label Nov 12, 2024
@pre-commit-ci pre-commit-ci bot removed the pre-commit.ci autofix run autofix in this PR label Nov 12, 2024
@fzimmermann89
Copy link
Member Author

needs #517 ..

@fzimmermann89
Copy link
Member Author

We could remove the Gram Operators and instead return directly the composition of the other operators.
But this might be a bit confusing, that FourierOp.gram results in a composition of 2 FourierOps, two multiplications, ...

@fzimmermann89
Copy link
Member Author

Also, it could be made a bit more efficient if we add a normalize and fftshift flag to the fftop, we can avoid both by scaling/shifting the kernel instead.

@fzimmermann89 fzimmermann89 changed the title Gram Implement Gram Shortcut for FourierOp Nov 13, 2024
@fzimmermann89 fzimmermann89 self-assigned this Nov 13, 2024
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

src/mrpro/operators/FourierOp.py Show resolved Hide resolved
src/mrpro/operators/FourierOp.py Outdated Show resolved Hide resolved
src/mrpro/operators/FourierOp.py Show resolved Hide resolved
@Stef-Martin
Copy link
Collaborator

Looks good to me as well, Toeplitz Kernel from tkbnufft and Gram also give very similar results

@fzimmermann89 fzimmermann89 enabled auto-merge (squash) November 14, 2024 12:35
@fzimmermann89 fzimmermann89 merged commit 96f66aa into main Nov 14, 2024
20 checks passed
@fzimmermann89 fzimmermann89 deleted the gram branch November 14, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants