FIX: Ensure dataobj datatype is altered #527
Conversation
|
Best reviewed: commit by commit
Optimal code review plan (1 warning)
|
|
This will have no effect. |
|
I don't fully understand the |
Codecov Report
@@ Coverage Diff @@
## master #527 +/- ##
==========================================
+ Coverage 64.08% 64.14% +0.05%
==========================================
Files 43 43
Lines 5288 5285 -3
Branches 773 769 -4
==========================================
+ Hits 3389 3390 +1
+ Misses 1754 1750 -4
Partials 145 145
Continue to review full report at Codecov.
|
|
It was meant to only effect the on-disk format. I'm a bit hesitant to coerce the data within the datasink interface, as there is already a lot going on there. If we want to go that route, I'd feel more comfortable splitting that into its own Node |
mgxd
changed the title
There was a problem hiding this comment.
I believe we also want to revise the aparcaseg resampling. This change is necessary (it doesn't make much sense not to cast the data array, despite of the risk of corrupting the data), but we need to make sure the datatype is correctly set on the resampled output.
niworkflows/interfaces/bids.py
Outdated
| f"Changing {out_file} dtype from {orig_dtype} to {data_dtype}" | ||
| ) | ||
| # coerce dataobj to new data dtype | ||
| new_data = np.asanyarray(nii.dataobj).astype(data_dtype) |
There was a problem hiding this comment.
| new_data = np.asanyarray(nii.dataobj).astype(data_dtype) | |
| new_data = np.asanyarray(nii.dataobj, dtype=data_dtype) |
There was a problem hiding this comment.
Is this going to cast BOLD data to int16?
There was a problem hiding this comment.
i think nipreps/smriprep#195 is part of the problem, but there is some further step downstream that is upcasting to float. But this is also necessary for any conversion to match source_file (currently just boldref).
There was a problem hiding this comment.
@effigies this should only affect data suffixes:
("mask", "uint8"),
("dseg", "int16"),
("probseg", "float32"),
("boldref", "source")
unless the data_dtype flag is explicitly checked, which I don't believe it ever is.
There was a problem hiding this comment.
i think poldracklab/smriprep#195 is part of the problem, but there is some further step downstream that is upcasting to float.
commented there, yes there is a resampling downstream in this case - moving to standard space.
Co-authored-by: Oscar Esteban <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
|
I kept running into: when trying to set Logically, it would have to round first and then apply the dtype to the array anyways. |
|
i'll merge this in now and cut a new release, thank you both for the reviews |
brought to attention in nipreps/fmriprep#2142