PTB-MR / mrpro

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

Add tests for autodiff of operators #405

Closed ckolbPTB closed 2 days ago

ckolbPTB commented 1 month ago

Adds the gradient checks for operators from #307

github-actions[bot] commented 1 month ago

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.py72297%118, 157
   ConstraintsOp.py60297%46, 48
   EndomorphOperator.py65297%228, 234
   FiniteDifferenceOp.py27293%40, 105
   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
TOTAL477535093% 

Tests Skipped Failures Errors Time
1989 0 :zzz: 0 :x: 0 :fire: 1m 56s :stopwatch:
ckolbPTB commented 1 month ago

The MOLLI test fails. Currently we have: signal(a,b,t1) = a * (1 - b/a * torch.exp(ti / t1 * (1 - b/a)))

The b/a term is what is causing problems. My suggestion would be to change this to signal(a,c,t1) = a * (1 - c * torch.exp(ti / t1 * (1 - c)))

a and b don't have any physiological meaning and this should also make it easier to find good constraints for c.

See #406

fzimmermann89 commented 1 month ago

remind me again, we we have no grad_check in the helpers, testing the numeric values of the gradients?

ckolbPTB commented 1 month ago

remind me again, we we have no grad_check in the helpers, testing the numeric values of the gradients?

I don't think we have discussed this yet. If we use pytorch autograd functionality, would this test not merely test pytorch functionality?

ckolbPTB commented 3 weeks ago

@fzimmermann89 could you have a look at this again please

github-actions[bot] commented 3 weeks ago

:books: Documentation

:file_folder: Download as zip :mag: View online

fzimmermann89 commented 3 weeks ago

So far, what is there look fine to me

ckolbPTB commented 3 days ago

remind me, what was the plan here? should we add a test for each operator and its adjoint?

With this PR we wanted to add autodiff tests for all non-linear operators. autodiff (and some more) tests for linear operators would then we added in PR #407

fzimmermann89 commented 2 days ago

Will need #515 to pass mypy

fzimmermann89 commented 2 days ago

Will need #515 to pass mypy

I stand corrected -- #515 would make the type hints more useful, but it passes without the changes