search

PTB-MR / mrpro

MR image reconstruction and processing.
https://ptb-mr.github.io/mrpro/
Apache License 2.0
17 stars 2 forks source link

Adapt it sense functions #379

Closed ckolbPTB closed 2 months ago

ckolbPTB commented 3 months ago

Here I changed the from_kdata such that also a CsmData can be provided and it is more flexible how the CsmData is calculated (assuming we will have espirit, inati... in the future). Especially for IterativeSENSE we probably will calculate the CSM before calling from_kdata.

I also separted the forward from creating H and b. Having one single function which returns (H, b) together would save a bit of duplicate code. Not sure which version I prefer...

github-actions[bot] commented 3 months ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms/csm
   inati.py24196%44
   iterative_walsh.py15193%37
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.py422345%77–78, 88–98, 113–124, 138–149
   Reconstruction.py512453%41, 53–55, 79–86, 103–114
src/mrpro/data
   AcqInfo.py128298%174, 214
   CsmData.py28389%14, 84–86
   DcfData.py37295%17, 65
   IData.py60395%119, 125, 129
   IHeader.py69297%76, 110
   KHeader.py1571292%23, 125–129, 156, 206, 217, 224–225, 228, 235
   KNoise.py24962%39–52
   MoveDataMixin.py1261489%14, 109, 125, 139–141, 202, 265, 279, 358, 378–379, 396–397
   QData.py32197%42
   SpatialDimension.py44198%64
   TrajectoryDescription.py14193%23
   acq_filters.py10190%47
src/mrpro/data/_kdata
   KData.py97991%107–108, 117, 125, 179–180, 215, 220–221
   KDataRemoveOsMixin.py29293%43, 45
   KDataSelectMixin.py20290%46, 62
   KDataSplitMixin.py48394%49, 79, 88
src/mrpro/data/traj_calculators
   KTrajectoryCalculator.py25292%23, 45
   KTrajectoryIsmrmrd.py13285%41, 50
   KTrajectoryPulseq.py29197%54
src/mrpro/operators
   CartesianSamplingOp.py50982%49–50, 55–56, 61–62, 88, 91, 114
   ConstraintsOp.py60297%46, 48
   EndomorphOperator.py51296%209, 213
   FiniteDifferenceOp.py27293%48, 113
   FourierOp.py77199%131
   GridSamplingOp.py123993%60–61, 70–71, 78–79, 82, 84, 86
   LinearOperator.py80495%32, 131, 251, 256
   Operator.py52198%21
   SliceProjectionOp.py166895%39, 46, 48, 54, 191, 212, 245, 285
   ZeroPadOp.py16194%30
src/mrpro/utils
   Rotation.py4532894%58–66, 106, 283, 368, 370, 397, 452, 457, 460, 475, 492, 497, 640, 645, 648, 664, 668, 742, 744, 752–753, 993, 1075
   filters.py62297%44, 49
   slice_profiles.py45687%18, 34, 111–114, 147
   sliding_window.py34197%34
   split_idx.py10280%43, 47
   zero_pad_or_crop.py31681%26, 30, 54, 57, 60, 63
TOTAL341822693% 

Tests Skipped Failures Errors Time
762 0 :zzz: 0 :x: 0 :fire: 1m 10s :stopwatch:
github-actions[bot] commented 3 months ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms/csm
   iterative_walsh.py15193%38
src/mrpro/algorithms/dcf
   dcf_voronoi.py53492%15, 48–49, 76
src/mrpro/algorithms/optimizers
   adam.py17194%64
src/mrpro/algorithms/reconstruction
   DirectReconstruction.py311648%40–45, 63–73, 87
   IterativeSENSEReconstruction.py593737%65–71, 96–106, 116–126, 141–152, 166–177
   Reconstruction.py502452%40, 52–54, 70–77, 94–105
src/mrpro/data
   AcqInfo.py123298%162, 200
   CsmData.py21386%14, 58–60
   DcfData.py37295%17, 65
   IData.py61395%124, 130, 134
   IHeader.py65297%76, 103
   KHeader.py1611293%23, 125–129, 164, 214, 225, 232–233, 236, 243
   KNoise.py24962%39–52
   MoveDataMixin.py1261489%14, 108, 124, 138–140, 201, 264, 278, 357, 377–378, 395–396
   QData.py32197%42
   SpatialDimension.py44198%64
   TrajectoryDescription.py14193%23
   acq_filters.py10190%47
src/mrpro/data/_kdata
   KData.py97991%107–108, 117, 125, 179–180, 215, 220–221
   KDataRemoveOsMixin.py29293%43, 45
   KDataSelectMixin.py20290%46, 62
   KDataSplitMixin.py48394%49, 79, 88
src/mrpro/data/traj_calculators
   KTrajectoryCalculator.py25292%23, 45
   KTrajectoryIsmrmrd.py13285%41, 50
   KTrajectoryPulseq.py29197%54
src/mrpro/operators
   CartesianSamplingOp.py50982%49–50, 55–56, 61–62, 88, 91, 114
   ConstraintsOp.py60297%46, 48
   EndomorphOperator.py51296%209, 213
   FiniteDifferenceOp.py27293%48, 113
   FourierOp.py77199%131
   GridSamplingOp.py122993%59–60, 69–70, 77–78, 81, 83, 85
   LinearOperator.py80495%32, 131, 251, 256
   Operator.py52198%21
   SliceProjectionOp.py166895%39, 46, 48, 54, 191, 212, 245, 285
   ZeroPadOp.py16194%30
src/mrpro/utils
   Rotation.py4532894%58–66, 106, 283, 369, 371, 398, 453, 458, 461, 476, 493, 498, 642, 647, 650, 666, 670, 745, 747, 755–756, 996, 1078
   filters.py61297%43, 48
   slice_profiles.py45687%18, 34, 111–114, 147
   sliding_window.py34197%34
   split_idx.py10280%43, 47
   zero_pad_or_crop.py31681%26, 30, 54, 57, 60, 63
TOTAL336823993% 

Tests Skipped Failures Errors Time
751 0 :zzz: 0 :x: 0 :fire: 1m 9s :stopwatch:
fzimmermann89 commented 3 months ago

My idea was to keep these somewhat simple and opinionated, as there is always the possibility to just the the reconstruction manually.

Also, the reconstruction can be initiated using the init with any csm.

I would oppose to having a if switch in from_kdata. Either do dependency inversion and allow to supply the CSM function somewhere. I would like to not have string literals to switch between different functionality.

Or just use the init, and have from_kdata choose some sensible defaults.

This PR moves in the wrong direction imho, as it blurs the lines between the different "levels" of how to do the reconstruction with mrpro.

Gesendet von Outlook für Androidhttps://aka.ms/AAb9ysg

From: Christoph Kolbitsch @.> Sent: Friday, August 2, 2024 9:03:57 PM To: PTB-MR/mrpro @.> Cc: Subscribed @.***> Subject: [PTB-MR/mrpro] Adapt it sense functions (PR #379)

Here I changed the from_kdata such that also a CsmData can be provided and it is more flexible how the CsmData is calculated (assuming we will have espirit, inati... in the future). Especially for IterativeSENSE we probably will calculate the CSM before calling from_kdata.

I also separted the forward from creating H and b. Having one single function which returns (H, b) together would save a bit of duplicate code. Not sure which version I prefer...

You can view, comment on, or merge this pull request online at:

https://github.com/PTB-MR/mrpro/pull/379

Commit Summary

File Changes

(2 fileshttps://github.com/PTB-MR/mrpro/pull/379/files)

Patch Links:

— Reply to this email directly, view it on GitHubhttps://github.com/PTB-MR/mrpro/pull/379, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABH3DVZLFEFJIZJDX6R5SA3ZPPJZ3AVCNFSM6AAAAABL5B4JNCVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ2DKNJZGQ4DGMY. You are receiving this because you are subscribed to this thread.Message ID: @.***>

ckolbPTB commented 3 months ago

Also, the reconstruction can be initiated using the init with any csm.

True, but I find from_kdata a very useful function and I would like to utilise it also as best as possible for the iterative reconstructions. Especially as we will also have to add CartesianSamplingOp the __init__ of each reconstruction if we want to enable Cartesian reconstructions.

One other option would be to leave DirectReconstruction.from_kdata() as it was before and change IterativeSENSEReconstruction.from_kdata(kdata: KData, csm: CsmData | None, noise: KNoise | None, n_iteartions: int), i.e. CSM needs to be provided. IMO this is the most useful case for this iterative recon.

fzimmermann89 commented 2 months ago

Just to summarize the discussion: The init should now have an additional argument kdata. If kdata is specified and none of the other arguments are specified, the former from_kdata logic should apply and everything should be estimated from kdata. If, for example, a fourieroperator is given, it will be used instead. If neither kdata nor a fouriroperator is given, an error is raised.

(I think @ckolbPTB already implementedthis in the last commits..)