Closed effigies closed 1 week ago
Attention: Patch coverage is 85.71429%
with 1 line
in your changes missing coverage. Please review.
Project coverage is 92.20%. Comparing base (
e6ccec4
) to head (9afa0be
). Report is 11 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
nibabel/nifti1.py | 85.71% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for implementing this. Anything you need from our end?
Not really, just a heads up that this would break any code that expects img.header.extensions[0].get_content()
to be bytes
instead of a dict
. To support multiple versions of nibabel, you could have a little function like:
def get_mrs_header(img: nb.Nifti1Image) -> dict | None:
exts = img.header.extensions.get_codes()
if 44 not in exts:
return None
mrs_header = img.header.extensions[exts.index(44)].get_content()
if isinstance(mrs_header, bytes): # nibabel < 6
return json.loads(mrs_header.decode())
return mrs_header
I think I'm leaning toward #1336 instead of this. In this case, you could continue to use json.loads(ext.get_content())
for older nibabel, or you could update to using ext.json()
instead once that patch is included in the oldest supported version of nibabel.
Superseded by #1336.
We haven't kept up with all the NIfTI extensions added over the years. With NIfTI-MRS being on the verge of adoption into BIDS, it seems like the least we can do to support JSON extensions.
I'm not sure if any of the other extensions are JSON, so I'm leaving them as bytes for now. I'm a bit inclined to abstract this out so that we can properly type these things, but we'll see how much I feel the need to procrastinate on more important things.
cc @wtclarke @markmikkelsen