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

FIX: Set MRS type to Nifti1Extension for backwards compatibility #1380

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

effigies
Copy link
Member

@wtclarke
Copy link

Does Nifti1Extension still have the .json() methods? I've just updated the nifti-mrs package to use the new interface. So if the new methods aren't available for this, then I'm happy to just make the changes on my side and not have this fix.

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.39%. Comparing base (96c8320) to head (56446e5).
Report is 5 commits behind head on maint/5.3.x.

❗ There is a different number of reports uploaded between BASE (96c8320) and HEAD (56446e5). Click for more details.

HEAD has 57 uploads less than BASE
Flag BASE (96c8320) HEAD (56446e5)
58 1
Additional details and impacted files
@@               Coverage Diff               @@
##           maint/5.3.x    #1380      +/-   ##
===============================================
- Coverage        95.38%   86.39%   -8.99%     
===============================================
  Files              207      207              
  Lines            29672    29665       -7     
  Branches          3018     3018              
===============================================
- Hits             28302    25629    -2673     
- Misses             931     3594    +2663     
- Partials           439      442       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Member Author

Yes, Nifti1Extension subclasses NiftiExtension[bytes] so _mangle() and _unmangle() are just identity functions, as was the previous default. The purpose of splitting the classes was so that new subclasses could derive from the base class and declare their types correctly.

@wtclarke
Copy link

Great, I see now, I was getting confused between Nifti1Extension and Nifti1Extensions. Thanks for the fix.

@effigies
Copy link
Member Author

I guess with the problem solved on your end, we can take a minute to think about how you would like to evolve.

If you would like to offload more of your logic into nibabel and use a NiftiExtension directly, then it's possible that creating a JSONExtension subclass would make more sense. That would allow ext.get_object() to return a cached value that you can mutate. When it's time to save to a new image, that object will override the original bytes content.

If read-only access is all you want, then this PR will at least get you back what you had, without users having to upgrade your package if they upgrade nibabel. In practice, though, I expect most people just install the latest versions, and so making this release or not will impact very few people.

@effigies
Copy link
Member Author

Well, regardless of the long-term path forward, this has the potential to reduce pain and doesn't make changing in the future harder.

@effigies effigies merged commit 7b5060e into nipy:maint/5.3.x Oct 23, 2024
6 of 21 checks passed
@effigies effigies deleted the fix/mrs_type branch October 23, 2024 13:33
@wtclarke
Copy link

Hi @effigies, Thanks for this fix. Sorry for the slow reply, four new starters in the group has me rushing about this week. For now I'm happy to keep the logic on my side.

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