FIX: Use seconds as time units when converting PARRECHeader to Nifti1Header#931
Conversation
|
Would be glad if anybody wants to weigh in on which of these two approaches is best. I think this one will specifically fix conversions and not break anybody's workflows that have been depending on |
Codecov Report
@@ Coverage Diff @@
## maint/3.1.x #931 +/- ##
===============================================
- Coverage 91.86% 91.77% -0.09%
===============================================
Files 97 97
Lines 12360 12360
Branches 2177 2177
===============================================
- Hits 11354 11344 -10
- Misses 675 681 +6
- Partials 331 335 +4
Continue to review full report at Codecov.
|
|
Anybody care to have a look? If not I'll exercise supreme maintainer powers and hit merge tomorrow. @Roosted7 @epongpipat I would appreciate your opinions on this one. |
|
Hmm - it's a tough call, I agree. In general, we try to report the file from nibabel as a native reader would have reported it, but here I guess we allowed ourselves to make the file appear more NIfTI-like. This fix seems better for back-compatibility, but you could also make an argument for converting to seconds only when converting the image to NIfTI format (so, not in |
|
How about going with this for a bug fix, and we can move PARRECImage to report msec TR in a feature release? Maybe it would be worth revisiting #567. I ran into some conceptual problems trying to think through the API there. |
|
Sounds reasonable ...
|
parrec2nii produces NIfTI files with TRs in seconds but records the units as msec
#692 proposed retaining the PAR/REC msec units by removing a division by
1000.. This PR instead proposes keeping things in seconds.cc @Roosted7 @epongpipat
Replaces #692.