Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

effigies
Copy link
Member

@effigies effigies commented Jun 9, 2024

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

Copy link

codecov bot commented Jun 9, 2024

Codecov Report

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 ⚠️
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.
📢 Have feedback on the report? Share it here.

@effigies effigies force-pushed the enh/mrs_extension branch from 81eba53 to 63f0647 Compare June 9, 2024 16:25
@wtclarke
Copy link

Thanks for implementing this. Anything you need from our end?

@effigies
Copy link
Member Author

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

@effigies
Copy link
Member Author

effigies commented Jul 6, 2024

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.

@effigies
Copy link
Member Author

Superseded by #1336.

@effigies effigies closed this Sep 24, 2024
@effigies effigies deleted the enh/mrs_extension branch September 24, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants