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 .content, .text and .json properties to NIfTI extensions #1336

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

effigies
Copy link
Member

@effigies effigies commented Jul 5, 2024

The NIfTI-MRS standard (https://github.com/wtclarke/mrs_nifti_standard/) includes a JSON extension. My initial attempt at this (in #1327) created a NiftiJSONExtension class, but this had the side-effect of breaking code that already loaded the extension as bytes and manually decoding.

This PR instead includes the following protocol:

class NiftiExtension:
    content: bytes
    text: str
    json: dict[str, Any]

This follows the protocol used by requests and httpx and assumes that most extensions will be sufficiently served by one or another of these three attributes (properties, actually), and mangling/unmangling is only needed for more intricate types, such as DICOM.

Nifti1Extension is a non-ideal base class for NIfTI extensions because it assumes that it is safe to use an identity transformation to convert between the on disk and in memory representation of the extension contents, and thus default to bytes objects. This makes it difficult to define its typing behavior in a way that allows subclasses to refine the type such that type-checkers understand it.

This patch creates a generic NiftiExtension class that parameterizes the "runtime representation" type. Nifti1Extension subclasses with another parameter that defaults to bytes, allowing it to be subclassed in turn (preserving the Nifti1Extension -> Nifti1DicomExtension subclass relationship) while still emitting bytes. Further, this allows extensions to be created with either bytes or an object (for subclasses that define a particular object type), avoiding unnecessary mangle/unmangle round-trips.

We could have simply made Nifti1Extension the base class, but the mangle/unmangle methods need some casts or ignore comments to type-check cleanly. This separation allows us to have a clean base class with the legacy hacks cordoned off into an subclass.

The Cifti2Extension needed very little updating.

Closes #1335.

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 96.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.34%. Comparing base (83eaf0b) to head (398488e).

Files with missing lines Patch % Lines
nibabel/nifti1.py 95.45% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1336      +/-   ##
==========================================
- Coverage   95.34%   95.34%   -0.01%     
==========================================
  Files         207      207              
  Lines       29507    29555      +48     
  Branches     4982     4993      +11     
==========================================
+ Hits        28134    28179      +45     
- Misses        932      935       +3     
  Partials      441      441              

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

Nifti1Extension is a non-ideal base class for NIfTI extensions because
it assumes that it is safe to store use a null transformation, and thus
default to `bytes` objects. This makes it difficult to define its typing
behavior in a way that allows subclasses to refine the type such that
type-checkers understand it.

This patch creates a generic `NiftiExtension` class that parameterizes
the "runtime representation" type. Nifti1Extension subclasses with
another parameter that defaults to `bytes`, allowing it to be subclassed
in turn (preserving the Nifti1Extension -> Nifti1DicomExtension subclass
relationship) while still emitting `bytes`.

We could have simply made `Nifti1Extension` the base class, but the
mangle/unmangle methods need some casts or ignore comments to type-check
cleanly. This separation allows us to have a clean base class with the
legacy hacks cordoned off into an subclass.
nibabel/nifti1.py Outdated Show resolved Hide resolved
@effigies effigies changed the title RF/TYP: Annotate NIfTI extensions RF/TYP: Add .content, .text and .json properties to NIfTI extensions Sep 8, 2024
nibabel/nifti1.py Outdated Show resolved Hide resolved
@effigies
Copy link
Member Author

A review would be appreciated here. I got back to this because I wanted to play around with serializing xibabel images to NIfTI, optionally dumping "unknown" metadata fields into a JSON extension, and this will make that more convenient.

Does this seem like a reasonable interface?

@effigies effigies changed the title RF/TYP: Add .content, .text and .json properties to NIfTI extensions ENH: Add .content, .text and .json properties to NIfTI extensions Sep 13, 2024
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.

NIfTI Extension redesign
1 participant