PTB-MR / mrpro

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

Fix errors due to pydicom 3.0 #391

Closed ckolbPTB closed 2 months ago

ckolbPTB commented 2 months ago

Pydicom 3.0 was released which led to several errors due to deprecation, see e.g. https://github.com/pydicom/pydicom/pull/1567/files#diff-9b5a2e48457143cde31b1bbe1c7546a6733e577c306cfe582d56099851e12468

github-actions[bot] commented 2 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.py44882%17, 65, 77–82
   IData.py67987%119, 125, 129, 159–167
   IHeader.py75791%75, 109, 127–131
   KHeader.py1641790%24, 126–130, 157, 207, 218, 225–226, 229, 236, 275–286
   KNoise.py311552%39–52, 56–61
   KTrajectory.py69593%178–182
   MoveDataMixin.py1261489%14, 109, 125, 139–141, 202, 265, 279, 358, 378–379, 396–397
   QData.py39782%42, 65–73
   SpatialDimension.py46296%64, 103
   TrajectoryDescription.py14193%23
   acq_filters.py10190%47
src/mrpro/data/_kdata
   KData.py1051685%107–108, 117, 125, 179–180, 215, 220–221, 240–251
   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
   summarize_tensorvalues.py11918%20–29
   zero_pad_or_crop.py31681%26, 30, 54, 57, 60, 63
TOTAL348728292% 

Tests Skipped Failures Errors Time
762 0 :zzz: 0 :x: 0 :fire: 1m 9s :stopwatch:
ckolbPTB commented 2 months ago

Currently all warnings are raised as errors in pytest (see https://github.com/PTB-MR/mrpro/blob/ca2f07b7030fe8ce3674f23207f974efcbc09fd3/pyproject.toml#L66)

This means that e.g DeprecationWarnings in other packages lead to errors. @fzimmermann89 @schuenke : Is this the behaviour we want, or should it only lead to errors for warning raised directly by our mrpro code?

fzimmermann89 commented 2 months ago

DeprecationWarnings as errors makes sense IMHO.

It would be better if we could somehow set a depth for this, i.e. only if we call deprecated code it should error, not if one of our dependencies calls a deprecated function. But I think that might not be possible to detect..

fzimmermann89 commented 2 months ago

(If third party code throws a DeprecationWarning that we cannot fix, we could add an ignore to the toml filter warning setting for this deprecating warning)

fzimmermann89 commented 2 months ago

@CodiumAI-Agent /review

CodiumAI-Agent commented 2 months ago

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/PTB-MR/mrpro/commit/6bbf37f3170b789183db2e41d140232d9a46afac)

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Type Consistency
The function `get_item` and `get_items_from_all_dicoms` have changed the type of the `name` parameter from `str | TagType` to `TagType` only. Ensure that this change does not break existing functionality where a string might be passed.
fzimmermann89 commented 2 months ago

Got it, TagType includes str

...and you already answered the question :D sorry

CodiumAI-Agent commented 2 months ago

Persistent review updated to latest commit https://github.com/PTB-MR/mrpro/commit/c51e21ff4796e14f19aa072ab56022a166fe9775

CodiumAI-Agent commented 2 months ago

Persistent review updated to latest commit https://github.com/PTB-MR/mrpro/commit/6bbf37f3170b789183db2e41d140232d9a46afac