FIX: Use BIDS metadata for TR over nii header #357
Conversation
|
LGTM, but are you sure we want to trust the JSON sidecar over the nifti header? I guess my naive expectation is that errors are likely to be more common in the former than the latter. |
|
@tyarkoni - my naive expectation would be the opposite since most people don't write these sidecars by hand, and i think the bids spec says somewhere that sidecar takes precedence. |
|
@satra sure, but I don't think most people are editing nifti headers by hand either, and if I had to guess, I think it's more likely that the sidecars are being set based on information extracted from the headers than the other way around. That said, if the spec says that the sidecar takes precedence, that's good enough for me. :) |
|
+1 on @satra - .json sounds like a more reasonable place to allow for overrides... |
bids/variables/io.py
Outdated
| @@ -147,7 +147,8 @@ def _load_time_variables(layout, dataset=None, columns=None, scan_length=None, | |||
| try: | |||
| import nibabel as nb | |||
| img = nb.load(img_f) | |||
| duration = img.shape[3] * img.header.get_zooms()[-1] | |||
| tr = layout.get_metadata(img_f)['RepetitionTime'] | |||
There was a problem hiding this comment.
what if it is not there?
The bible describes it as REQUIRED but then it says that it is "mutually exclusive" with VolumeTiming. I do not think that if a varying VolumeTiming is supported anywhere here but then it either must blow with a meaningful message if RepetitionTime is absent or look into VolumeTiming and verify that it is equispaced/deduce the TR, or fallback to lookup in .nii.gz
This echoes with another comment +1'ing Satra that .json ideally takes precendence since apparently with VolumeTiming it might be quite obscure case and pybids better blows if finds notably variable timing instead of plowing through
PS FWIW, I saw no dataset with VolumeTiming in metadata extracted from openneuro datasets, so it is more of a "future proofing" I guess
|
I agree with @satra, though I'm not sure the spec indicates precedence. My reasoning is that JSON is easier to inspect and fix than NIfTI headers if there are problems, and therefore should be presumed to indicate intent with higher reliability. I'll also note that at line ~162, we do pull this TR out of the metadata just after this point, so we should probably be consistent about which we give precedence, regardless of the decision. I would suggest we just move that So don't merge this yet. I'll try to find a couple minutes to clean things up. Also, I agree with @yarikoptic that we should check on |
|
😈 advocate counter argument: other/basic tools have no clue about overrides in json and operate on information from .nii - so we would be running into the danger of obtaining inconsistent or wrong results if we allow such overrides in json. I think that we actually should explicitly in bids enforce consistency between json and .nii.gz . And iirc bids validator already does that right? |
|
To be clear, I have no problem at all with assuming that JSON overrides metadata extracted from other sources in pybids. It's a simple principle that's easy to implement. I'm happy with it. My question was meant more for my own personal edification—I was just curious whether, implementation aside, we should expect the nifti header to be more accurate than the JSON. Nothing important hangs on it one way or the other, so I suggest we go with the proposal here, and move on. |
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
==========================================
+ Coverage 73.27% 73.28% +0.01%
==========================================
Files 24 24
Lines 2604 2605 +1
Branches 640 640
==========================================
+ Hits 1908 1909 +1
Misses 513 513
Partials 183 183
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
==========================================
+ Coverage 73.27% 73.28% +0.01%
==========================================
Files 24 24
Lines 2604 2605 +1
Branches 640 640
==========================================
+ Hits 1908 1909 +1
Misses 513 513
Partials 183 183
Continue to review full report at Codecov.
|
|
Okay, this is ready for merge, IMO. |
|
Would it be useful to open an issue at https://github.com/bids-standard/bids-specification about the precedence? Or is it already clear? (This is orthogonal to merging the PR - just thinking about avoiding others having the same conversation!) |
|
I agree. Thanks for the prod, @KirstieJane. If anybody wants to continue the discussion, let's have it on bids-standard/bids-specification#138. |
Kind of fixes #355. Turns out the nifti (and BIDS metadata) had a TR of 2.00001 (or thereabouts), which caused a misestimation of duration, which caused one extra volume on a long time series.
This fix allows the change in the JSON sidecar (easier than editing NIFTI headers) to take effect. We should also generally prefer JSON metadata to NIFTI headers, anyway.