-
Notifications
You must be signed in to change notification settings - Fork 257
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
ENH: Add NiftiJSONExtension class, use for NIfTI-MRS #1327
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1327 +/- ##
=======================================
Coverage 92.19% 92.20%
=======================================
Files 98 98
Lines 12397 12404 +7
Branches 2557 2556 -1
=======================================
+ Hits 11430 11437 +7
Misses 644 644
Partials 323 323 ☔ View full report in Codecov by Sentry. |
81eba53
to
63f0647
Compare
Thanks for implementing this. Anything you need from our end? |
Not really, just a heads up that this would break any code that expects 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 |
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