search

nipy / heudiconv

Flexible DICOM conversion into structured directory layouts
https://heudiconv.readthedocs.io
Other
232 stars 125 forks source link

Replace deprecated "_phase" suffix in reproin heuristic with "part-phase" entity #770

Open tsalo opened 2 months ago

tsalo commented 2 months ago

Closes #769. This is working on DICOMs associated with https://openneuro.org/datasets/ds005250.

Changes proposed:

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.15%. Comparing base (a0a3635) to head (59bb2ec).

Files Patch % Lines
heudiconv/heuristics/reproin.py 90.90% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #770 +/- ## ========================================== + Coverage 82.08% 82.15% +0.06% ========================================== Files 42 42 Lines 4215 4230 +15 ========================================== + Hits 3460 3475 +15 Misses 755 755 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yarikoptic commented 2 weeks ago

Rename s variable to curr_seqinfo for improved clarity.

yeah, but complicates review a little due to diff now including changes I can ignore or not and need to decide when looking...

Could you submit a separate PR with that typo fix and such a rename (so no functionality change) and then let's improve this one -- yet to grasp on all the changes, and as we do not have any nice testing for reproin heuristic, I wonder how we could add one :-/ I would really be shy to accept without some way to test. Locally I could do using smth like https://github.com/nipy/heudiconv/blob/master/utils/test-compare-two-versions.sh but could we do better?!

tsalo commented 2 weeks ago

:+1: I've opened #779 with the variable name change.