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

Merged
merged 13 commits into from
Sep 23, 2024

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).
Report is 55 commits behind head on master.

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.

@effigies effigies force-pushed the rf/nifti_extensions branch 2 times, most recently from ba912d4 to e3437d2 Compare July 9, 2024 12:46
@effigies effigies force-pushed the rf/nifti_extensions branch from e3437d2 to 46923c1 Compare September 7, 2024 16:10
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.
@effigies effigies force-pushed the rf/nifti_extensions branch from 46923c1 to 061fbf5 Compare September 7, 2024 23:50
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
@effigies effigies merged commit c742552 into nipy:master Sep 23, 2024
49 checks passed
@effigies effigies deleted the rf/nifti_extensions branch September 23, 2024 13:47
effigies added a commit to effigies/nibabel that referenced this pull request Oct 15, 2024
nipygh-1336 reused the private attribute ``ext._content`` to exclusively
refer to the ``bytes`` representation of the extension contents.
This neglected that subclasses might depend on this implementation
detail.

Let's be nice to people and rename the attribute to ``_raw`` and provide
a ``_content`` property that calls ``self.get_content()``.

Also adds a test to ensure that multiple accesses continue to work as
expected.
effigies added a commit to effigies/nibabel that referenced this pull request Oct 15, 2024
nipygh-1336 reused the private attribute ``ext._content`` to exclusively
refer to the ``bytes`` representation of the extension contents.
This neglected that subclasses might depend on this implementation
detail.

Let's be nice to people and rename the attribute to ``_raw`` and provide
a ``_content`` property that calls ``self.get_content()``.

Also adds a test to ensure that multiple accesses continue to work as
expected.
effigies added a commit to effigies/nibabel that referenced this pull request Oct 15, 2024
nipygh-1336 reused the private attribute ``ext._content`` to exclusively
refer to the ``bytes`` representation of the extension contents.
This neglected that subclasses might depend on this implementation
detail.

Let's be nice to people and rename the attribute to ``_raw`` and provide
a ``_content`` property that calls ``self.get_content()``.

Also adds a test to ensure that multiple accesses continue to work as
expected.
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