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

🔧 Loaders and util for H5 and NIfTI transforms (propagation of contributions by @sgiavasis) #213

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

Conversation

jmarabotto
Copy link
Contributor

@jmarabotto jmarabotto commented Jul 19, 2024

(Propagation of @sgiavasis 's PR to @jmarabotto 's fork. Original PR message copied below)

Started at the NiPreps hackathon at MIT in May 2024.
Thanks again to @jmarabotto and @oesteban + @effigies for catching me up to speed and helping orient me on the nitransforms latest.

✅ This PR:

Adds loaders for .h5 and NIfTI (.nii or .nii.gz) transform files.
Adds a function for .x5 reading which only calls the H5 loader for now; set to be expanded as needed.
Adds an xfm_util level to the CLI which will read in a transform file and internally represent it as an instance of the TransformBase class, forming the X5 structure.
For now, this simply prints out some information, which can be used to verify the transform was read in properly.
Moves all I/O and I/O-related package/API calls to nitransforms/io/base.py
Starts the process of keeping the internal logic of the X5 transform structures and functions clean so that it can be unit tested without any actual transform files.
⚠️ Needs attention:

Functions exist to write out the X5 structure/transform information to either HDF5 and X5 files, but these need to be updated and completed.
More cleanup of dependencies on the lower-level details from the higher-level logic.
❓ Questions:

I don't think the internal structure of the X5 hierarchy is complete or accurate. I started with @jmarabotto 's great flowchart as a guide, but I know this might have been in flux.
In keeping with the flat namespace of the HDF5/X5 structure, the key levels remain flat (i.e. TransformGroup/0/Transform but I'm not sure if some of the final levels, like /Transform, should be a new sub-level/sub-dictionary or if they should directly contain the NumPy array defining the actual transform, etc.
▶️ Next steps (that I have in mind but I defer to the nitransforms contributors):

Completion of the write-out functions.
Some validation showing that converting any transform to an .x5 transform and applying it produces the exact same result as using the original (ex. ANTs, FSL etc.) transform with its appropriate application tool.

EDIT (OE, 08/02/2024) Just to keep these around:

@jmarabotto jmarabotto changed the title Enh/x5 implementation 🔧 Loaders and util for H5 and NIfTI transforms (propagation of contributions by @sgiavasis) Jul 19, 2024
@jmarabotto jmarabotto closed this Jul 24, 2024
@jmarabotto jmarabotto deleted the enh/x5-implementation branch July 24, 2024 09:27
@oesteban
Copy link
Collaborator

hey @jmarabotto, I believe you have mistakenly deleted this branch!

@jmarabotto jmarabotto restored the enh/x5-implementation branch July 24, 2024 16:36
@oesteban oesteban reopened this Jul 24, 2024
Comment on lines +255 to +259
"_affine",
"_shape",
"_header",
"_grid",
"_mapping",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is looking like TransformBase is becoming an affine transform. I will try to get it back to a generic, abstract base class.

Comment on lines +264 to +274
x5_struct = {
"TransformGroup/0": {
"Type": None,
"Transform": None,
"Metadata": None,
"Inverse": None,
},
"TransformGroup/0/Domain": {"Grid": None, "Size": None, "Mapping": None},
"TransformGroup/1": {},
"TransformChain": {},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to make this struct some sort of config option. Is there such a concept in nibabel, @effigies?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, but what kind of config option are you thinking? I'm very wary of putting mutable global state in a library, as that just asks for nasty interactions with multiple libraries (and threads). We could think about making a context manager to update state, along with ContextVars to manage state, if needed.

But possibly you're talking about something else? This looks like a place for a schema, but I haven't studied this code yet.

Comment on lines +292 to +305
if nifti is not None:
self._x5_dct = self.init_x5_structure(nifti)
elif hdf5:
self.update_x5_structure(hdf5)
elif x5:
self.update_x5_structure(x5)
self._shape = shape
self._affine = affine
self._header = header

# TO-DO
self._grid = None
self._mapping = None

Copy link
Collaborator

Choose a reason for hiding this comment

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

To linear transforms base

Comment on lines +402 to +430
def read_x5(self, x5_root):
variables = {}
with h5py.File(x5_root, "r") as f:
f.visititems(
lambda filename, x5_root: self._from_hdf5(filename, x5_root, variables)
)

_transform = variables["TransformGroup/0/Transform"]
_inverse = variables["TransformGroup/0/Inverse"]
_size = variables["TransformGroup/0/Domain/Size"]
_map = variables["TransformGroup/0/Domain/Mapping"]

return _transform, _inverse, _size, _map

def _from_hdf5(self, name, x5_root, storage):
if isinstance(x5_root, h5py.Dataset):
storage[name] = {
"type": "dataset",
"attrs": dict(x5_root.attrs),
"shape": x5_root.shape,
"data": x5_root[()], # Read the data
}
elif isinstance(x5_root, h5py.Group):
storage[name] = {
"type": "group",
"attrs": dict(x5_root.attrs),
"members": {},
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go into specific loaders :)

@@ -47,8 +49,43 @@ def cli_apply(pargs):
cval=pargs.cval,
prefilter=pargs.prefilter,
)
moved.to_filename(pargs.out or f"nt_{os.path.basename(pargs.moving)}")
# moved.to_filename(pargs.out or f"nt_{os.path.basename(pargs.moving)}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# moved.to_filename(pargs.out or f"nt_{os.path.basename(pargs.moving)}")
moved.to_filename(pargs.out or f"nt_{os.path.basename(pargs.moving)}")

Copy link
Collaborator

Choose a reason for hiding this comment

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

skipping this file for now

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.

4 participants