-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop functions to read sidecar JSONs #760
Conversation
I haven't removed |
@@ -115,7 +114,9 @@ def init_merge_and_denoise_wf( | |||
bids_dwi_files=raw_dwi_files, | |||
b0_threshold=config.workflow.b0_threshold, | |||
harmonize_b0_intensities=not config.workflow.no_b0_harmonization, | |||
scan_metadata={scan: get_metadata_for_nifti(scan) for scan in raw_dwi_files}, | |||
scan_metadata={ | |||
scan: config.execution.layout.get_metadata(scan) for scan in raw_dwi_files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue is that read_nifti_sidecar
is still used in places. It looks like there are a few interfaces/functions (get_distortion_grouping
and get_best_b0_topup_inputs_from
) that use read_nifti_sidecar
and do some complicated stuff, so I didn't try to remove it.
The readthedocs error is very strange, I don't think this PR changes anything in extras_require |
No, but I'm seeing the same failure in #766. I think it might be an issue with DiPy. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #760 +/- ##
==========================================
+ Coverage 29.76% 29.84% +0.07%
==========================================
Files 97 97
Lines 14586 14604 +18
Branches 1887 1885 -2
==========================================
+ Hits 4342 4359 +17
+ Misses 10118 10117 -1
- Partials 126 128 +2 ☔ View full report in Codecov by Sentry. |
I think this will work, but I'm wondering if we should hold off on major refactors until we add a couple more CI tests (like for GRE fieldmaps). Wdyt? |
I'm happy to wait on it. |
Closes #759.
Changes proposed in this pull request
qsiprep.interfaces.bids.ReadSidecarJSON
(this was actually unused, as the interface that was used ininit_phdiff_wf
was the one imported from niworkflows).qsiprep.interfaces.bids.get_metadata_for_nifti
in favor ofconfig.execution.layout.get_metadata
.ReadSidecarJSON
calls in favor ofconfig.execution.layout.get_metadata
.phase_meta
input toinit_phdiff_wf
. This actually matches the SDCFlows workflow a bit better as an added bonus.