Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
+ Coverage 70.35% 75.39% +5.04%
==========================================
Files 19 21 +2
Lines 1167 1463 +296
Branches 228 304 +76
==========================================
+ Hits 821 1103 +282
+ Misses 284 266 -18
- Partials 62 94 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) | ||
| nifti2_to_mif(temp_nifti2, out_mif) | ||
| result_header = template_img.header.copy() | ||
| result_header.set_data_shape(result_data.shape) |
There was a problem hiding this comment.
will the template ever be scaled? Codex review said this, not sure if accurate: scaled template MIFs will produce numerically wrong outputs. src/modelarrayio/cli/h5_to_mif.py (line 68) copies the template header verbatim, while src/modelarrayio/utils/mif.py (line 582) has already applied header scaling to the in-memory data and the writer (line 395) writes the same scaling metadata back out. Any non-default scaling in example_mif will be applied twice.
There was a problem hiding this comment.
I know very little about the MIF format. @mattcieslak do you know the answer to this?
tien-tong
left a comment
There was a problem hiding this comment.
src/modelarrayio/utils/mif.py is now doing three jobs at once: format parsing, nibabel image behavior, and cohort/fixel helpers. Splitting that module would make future review and testing much easier. The duplicated result-writing blocks in h5_to_mif.py (line 62) could also be factored into one helper.
Documentation: docs are stale. docs/installation.rst (line 17) still says MRtrix mrconvert is required, and docs/examples/plot_fixel_workflow.py (line 133) still says existing files are not overwritten.
|
@tien-tong I think I've addressed all of your comments except for the one on scaling. |
|
Looks good to me! |
mattcieslak
left a comment
There was a problem hiding this comment.
We should actually test this with some real fixel data, but I think it's pretty solid
Closes #15. Closes #17.
Keeping as a draft for now.
I've opened nipy/nibabel#1489 to add the MifImage class to nibabel. Depending on how quickly that PR progresses, we might want to keep copies of the class in ModelArrayIO for a release or two.
NOTE: Since HBCD does not include fixel data, this is a low priority item.