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 arithmetic to SpatialDimension #417

Closed fzimmermann89 closed 3 weeks ago

fzimmermann89 commented 1 month ago

Follow up to #416 with correct type hints

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.py422345%77–78, 88–98, 113–124, 138–149
   Reconstruction.py522454%42, 54–56, 80–87, 104–115
src/mrpro/data
   AcqInfo.py128298%174, 214
   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.py1651790%25, 127–131, 158, 210, 221, 228–229, 232, 239, 278–289
   KNoise.py311552%39–52, 56–61
   KTrajectory.py69593%178–182
   MoveDataMixin.py1271886%15, 110, 126, 140–142, 203, 265–267, 280, 359, 379–380, 382, 397–398, 400
   QData.py39782%42, 65–73
   Rotation.py6743695%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, 1608, 1690, 1846, 1881, 1885, 1996
   SpatialDimension.py2462391%34, 104, 119, 126, 132, 167, 177, 294–296, 309–311, 345, 363, 376, 389, 402, 415, 424–425, 440, 449
   TrajectoryDescription.py14193%23
   acq_filters.py10190%47
src/mrpro/data/_kdata
   KData.py1051685%108–109, 118, 126, 180–181, 216, 221–222, 241–252
   KDataRemoveOsMixin.py29293%44, 46
   KDataSelectMixin.py21290%48, 64
   KDataSplitMixin.py49394%51, 81, 90
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.py65297%228, 234
   FiniteDifferenceOp.py27293%40, 105
   FourierOp.py77199%131
   Functional.py71593%20–22, 117, 119
   GridSamplingOp.py136993%72–73, 82–83, 90–91, 94, 96, 98
   LinearOperator.py1611293%55, 91, 190, 220, 261, 270, 278, 287, 295, 306, 404, 409
   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
   modify_acq_info.py17194%12
   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
TOTAL453035192% 

Tests Skipped Failures Errors Time
1916 0 :zzz: 0 :x: 0 :fire: 1m 37s :stopwatch:
fzimmermann89 commented 1 month ago

I also moved Rotation out of utils (as it depended on SpatialDimension) and TYPE_CHECKING guarded modify_acq_info to avoid circular imports. Functions in utils should not depend on data classes (otherwise, we can't use them in data classes) As modify_acq_info will be deprecated anyways once we move that functionality into acqinfo, I only added the future annotations and guard.

These changes might be moved to a separate (stacked) PR .

I removed xyz to force us to always be zyx or be explicit about it and write (spatialdim.x,spatialdim.y,spatialdim.z). Otherwise, the temptation to wrongly use xyz is high..

fzimmermann89 commented 1 month ago

This passes all tests from #416 and passes mypy

fzimmermann89 commented 1 month ago

(To be merged after #418 into main)

github-actions[bot] commented 1 month ago

:books: Documentation

:file_folder: Download as zip :mag: View online

fzimmermann89 commented 1 month ago

i cleaned the type hints a bit.

also, I removed the conversion option from the class methods. Instead, there is now an apply_ that is used after creation to do the conversion. This will anyways be required for rearranging etc..

fzimmermann89 commented 1 month ago

Also, I added typing asserts as tests. This is in the same test_spatialdimension.py file, even though the tests is not actually run by pytest -- just parsed by mypy.

maybe @schuenke can have another look ...

ckolbPTB commented 4 weeks ago

Why do we want to support np.ndarray for SpatialDimension? It would be nice if that would be supported in the init and maybe from_-functions but it could then be converted to tensors.

fzimmermann89 commented 4 weeks ago

Why do we want to support np.ndarray for SpatialDimension? It would be nice if that would be supported in the init and maybe from_-functions but it could then be converted to tensors.

from_array* will convert to tensors. I mostly added nd.array type hints to see if the hints are correct or if errors are just hidden by the fact that there is only one vector type supported. The biggest uses I see is for compatibility and object arrays (example: array of callables). I would leave it in as long as it doesn't cause issues.

ckolbPTB commented 4 weeks ago

I would leave it in as long as it doesn't cause issues.

It seems strange to me why we would support ndarrays here when everywhere else we ensure torch.tensors. I would go the other way and remove it unless we really need it. Otherwise we should also support it for getitem/setitem

fzimmermann89 commented 4 weeks ago

Otherwise we should also support it for getitem/setitem

I added the overloads for getitem and setitem and a test for these.

fzimmermann89 commented 3 weeks ago

Otherwise we should also support it for getitem/setitem

I added the overloads for getitem and setitem and a test for these.

Scratch that (Was kümmert mich mein Geschwätz von gestern). I removed np.ndarray support, as I wont use a SpatialDimension of callables in the use case I had in mind anyway.

fzimmermann89 commented 3 weeks ago

pre-commit.ci autofix

fzimmermann89 commented 3 weeks ago

pre-commit.ci autofix