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

Specification draft #1

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Specification draft #1

wants to merge 35 commits into from

Conversation

balbasty
Copy link
Collaborator

Work-in-progress

python/setup.cfg Outdated Show resolved Hide resolved
@balbasty balbasty marked this pull request as draft December 16, 2023 23:13
@balbasty balbasty marked this pull request as ready for review December 16, 2023 23:14
@balbasty
Copy link
Collaborator Author

I have moved the python implementation to its own branch (PR #2), so that this PR is only concerned with the specification.

@satra
Copy link
Collaborator

satra commented Jan 12, 2024

inviting @effigies and @yarikoptic for comments. there are two other PRs open for the reference implementations in python and julia.

| `complex128` | `1792` | `"c16"` |
| `complex256` | `2048` | `"c32"` |
| `rgba32` | `2304` | <code>[["r", "&vert;u1"], ["g", "&vert;u1"], ["b", "&vert;u1"], ["a", "&vert;u1"]]</code> |
| `bool` | 🛑 unsupported! | <code>"&vert;b1"</code> |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

via nifti1.h

#define DT_BINARY                  1     /* binary (1 bit/voxel)         */
Suggested change
| `bool` | 🛑 unsupported! | <code>"&vert;b1"</code> |
| `bool` | `1` | <code>"&vert;b1"</code> |

Not to say it's not a terrible idea to try to use it...

Comment on lines +262 to +265
The nifti header ([v1](https://nifti.nimh.nih.gov/pub/dist/src/niftilib/nifti1.h)
or [v2](https://nifti.nimh.nih.gov/pub/dist/doc/nifti2.h)) **MUST** be encoded as
a string using [base64](https://en.wikipedia.org/wiki/Base64) encoding and saved in
the group-level `.zattrs` under the `["nifti"]["base64"]` key:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested in bids-standard/bids-specification#1704, I would consider saving the header as a binary array inside the base group instead of a JSON metadata field inside zattrs. A NIfTI-1 header could be a scalar array of type |S348 (np.array(img.header.binaryblock)) or an array of bytes 348 long (np.frombuffer(img.header.binaryblock, dtype='u1')).

Do you have any thoughts on handling NIfTI extensions?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part of the challenge is the interplay between ome-zarr and nifti-zarr. as long as we avoid those we should be ok. perhaps just call the key niftiheader. i'm sure @balbasty can come up with some specific form that satisfies both constraints :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like saving the binary block as an array! We'd indeed have to check that it does not violate OME validation.

For extensions, we could allow the binary block to contain everything up to the image data. It would mean that the header can be 348 or 352 or 352 + extension_size bytes. In that case, probably better to have the array be of type u1 than S348. We should also probably require that no compression be used for this array.

Do we have to store it in nifti/0/ or could it go directly in nifti/?

Would we still keep a JSON version of the header in .zattrs?

Do we want to formalize the conversion of nifti extensions to JSON?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything in the OME-Zarr spec prohibiting additional named arrays, but I have not played with OME-Zarr tooling to see if it would ignore such an array.

I think it matters very little whether you use |S348 or u1 for your dtype, as the |S<N> is automatically calculated from np.array(binaryblock).

For extensions, including them directly in the header seems fine to me, and consistent with treating nifti/0 as a .hdr file. I wasn't sure if you might want to do something a little more involved and split them, e.g.,

nifti/
  .zgroup
  header/
    0
    .zarray
  extensions/
    .zgroup
    0/
      0
      .zarray
    [...]
    n/
      0
      .zarray

This could have utility for manipulating individual extensions (for example, the AFNI extension has an appendable log) without forcing others to be shifted. But headers are the cheap part of the file, so this probably isn't worth worrying about.

Do we have to store it in nifti/0/ or could it go directly in nifti/?

From the API, you can use /nifti, but zarr will still need to create a chunk file 0, e.g.:

img.nii.zarr
├── 0
│   ├── 0.0.0
│   ├── [...]
│   ├── 3.3.3
│   └── .zarray
├── nifti
│   ├── 0
│   └── .zarray
└── .zgroup

Would we still keep a JSON version of the header in .zattrs?

I would personally prefer not to. A single source of truth that everybody already knows how to parse seems preferable to a JSON source that needs to be validated as close enough to the header.

@effigies
Copy link

The discussion of extensions makes me think you may want to consider versioning nifti-zarr itself and recording the version in the root .zattrs. I don't think it would be a good use of time to try to create a future proof layout now, but if it ever becomes worth revisiting, a version number that could be incremented would be nice.

@balbasty
Copy link
Collaborator Author

balbasty commented Mar 26, 2024

I've implemented the new specification that uses a "nifti" zarr-array in new "0.2" branches. See differences between 0.1 and 0.2 here:

I've also checked that nifti-zarr files are valid ome-zarr files using their online validator.

I am not sure what's the best way to proceed, open a new PR? I think the remaining questions are:

  1. Should we keep an optional JSON version of the nifti header? I currently do, in nifti/.zattrs. It brings readability but also induces information duplication.
  2. Should we version nifti-zarr, and if yes, how? Add a "version" string in nifti/.zattrs? There's a risk it would be confused with the version of the nifti header (we currently accept both nifti1 and nifti2 headers, and have the "magic" key in the JSON header).
  3. Should we support nifti's BINARY data type? I am not sure how to make it compatible with zarr. My understanding is that "|b1" still uses one byte per voxel, not one bit per voxel.
  4. Make sure we didn't overlook any potential side effect.
  5. Decide if we choose to stick with ome-ngff 0.4 forever, or if we want to support newer versions (and their transforms specification) in the future.

Edit: The julia implementation has now been updated. Note that neither the python nor julia implementation currently handle extensions.

@yarikoptic
Copy link

Should we version nifti-zarr, and if yes, how?

I guess you mean schema of the ".nifti" within .zattrs, right? then why not to store there explicitly as nifti-zarr-schema-version? May be even add some nifti-zarr-schema-url to point to the jsonschema for it if there any?

It brings readability but also induces information duplication.

a job for validator. It is unfortunate that AFAIK there is no yet universal support for extending zarr validators... FWIW I am chiming in at

which might be of interest (even if to bring my idea down ;) ).

@balbasty
Copy link
Collaborator Author

Yes, although I am also thinking of @effigies' idea of splitting out header extensions into multiple arrays under a nifti group, which could be added in a potential v2, if the community felt like it.

@fangq
Copy link

fangq commented Apr 21, 2024

@yarikoptic mentioned this thread to me.

I just want to point out that JSON wrapper of NIFTI has already existed via my JNIfTI spec since 2019, and has been extensively used in our NoSQL database based data portal NeuroJSON.io for in-browser rendering, for example

https://neurojson.org/db/abide2/ABIDEII-BNI_1#highlight=NIFTIHeader

you could also click on any of the purple-colored links in the "External data" section of each dataset to render JNIFTI data volumes in browser, for example

https://neurojson.org/db/adhd200/Brown

lightweight parsers in matlab/octave/python/javascript for reading/writing jnifti files already exist, and it support multiple compression codecs, such as the blosc2 codecs I mentioned in bids-standard/bids-specification#1614 (comment). We have a poster about JNIFTI in OHBM 2022

NeuroJSON_JNIfTI

Data grouping via NIFTIGroup tag is also defined in the JNIFTI spec

https://github.com/NeuroJSON/jnifti/blob/master/JNIfTI_specification.md#data-organization-and-grouping

at least for the NIFTI header metadata part, it would be nice not duplicating efforts - converting between different metadata conventions can really be frustrating to developers.

@balbasty
Copy link
Collaborator Author

Thanks @yarikoptic for connecting, and thanks @fangq for reaching out!

It does absolutely make sense to use the jnifti spec for the json version of the header. I guess the most straighforward way of doing this would be to make nifti/.zattrs a valid jnifti file (without the "NIFTIData" key).

Have you had a look at the rest of the spec? Your input would be appreciated. I would imagine you'd be a proponent of getting rid of the binary header altogether and use a jnifti only header? Our initial idea was that shipping the binary header required even less work for developers, who most likely already implement a parser for it.

@fangq
Copy link

fangq commented Apr 23, 2024

Thanks @yarikoptic for connecting, and thanks @fangq for reaching out!
It does absolutely make sense to use the jnifti spec for the json version of the header. I guess the most straighforward way of doing this would be to make nifti/.zattrs a valid jnifti file (without the "NIFTIData" key).

that would be fantastic!

Have you had a look at the rest of the spec? Your input would be appreciated.

I will take a deeper look.

one think I noticed is on the dimension orders
https://github.com/neuroscales/nifti-zarr/tree/draft/0.2?tab=readme-ov-file#3-main-differences-with-nifti-andor-ome-ngff

in jnifti's NIFTIHeader.Dim, we use nifti's native order x,y,z,c,t dimensions, but NIFTIData (which is a JData ND-array) data buffer uses C-order, so we have to do a permutation of the dimensions, or use "_ArrayOrder_" : "col" in the NIFTIData JSON construct.

I understand that you only going to need NIFTIHeader here, just want to point out the order used in NIFTIHeader is x,y,z,c,t. JNIFTI does not limit the dimensions to 5 (or 7), but if reversed conversion is expected, then it makes sense to set that cap.

please also take a look at my spec and see anything you expect discrepancies.

I would imagine you'd be a proponent of getting rid of the binary header altogether and use a jnifti only header? Our initial idea was that shipping the binary header required even less work for developers, who most likely already implement a parser for it.

regarding the binary header, in my matlab implementation, I am currently using the following two functions to dynamically convert jnifti header to a valid nifti-1/2 header

https://github.com/NeuroJSON/jnifty/blob/master/jnii2nii.m
https://github.com/NeuroJSON/jnifty/blob/master/nifticreate.m

(and the reversed direction is done in https://github.com/NeuroJSON/jnifty/blob/master/niiheader2jnii.m)

this way, we don't need to maintain two buffers with duplicated information, which is a risk for discrepancy - for example, if someone modified the jnifti header, the binary buffer must also be synchronized to be consistent if you have the info in two places.

I am in the process of translating my matlab functions to Python, happy to share once it is done. also feel free to port if it is easier for you to do it.

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.

5 participants