ENH: Define Cifti2 Axes describing the rows/columns of the Cifti2 data #641
Conversation
|
@MichielCottaar great that you did this! Let's start working on this! |
|
I think we can also merge this utils.py from my branch which allows for a better manipulation of indices. What do you think? |
|
Thanks, Demian. Your MatrixIndexMap seems quite similar to my Axis class, although rather than using sub-classing to support the various Cifti2 axes types you handle the various types through an if-statement in the init. So I would view it as an alternative, simpler implementation for if people think that my implementation is too complex. |
|
You are right! I didn't look in detail your code yet. I will go over the interfaces more than the implementation and check their simplicity. You code looks very nice at first sight! |
|
Hi @MichielCottaar @demianw, what's the status of this? I'm inclined to try to release soon, so if this isn't too far away, I can put it in the queue. |
|
Hey! I meant to do a code review months ago and I was buried in work. I expect to be able to review by nov 1st the latest. Does that sound good? |
|
Sounds good, but I'll plan to release before that. I think it would be good to establish a consistent release cycle (e.g., every 3 months no matter what), so that we don't have these weird, aperiodic merge windows that are too short for serious work to get done in. |
nibabel/cifti2/cifti2_axes.py
Outdated
| (N, ) structured array with for every element a tuple with 3 elements: | ||
| - vertex index (-1 for voxels) | ||
| - 3 voxel indices (-1 for vertices) | ||
| - string (name of brain structure) |
There was a problem hiding this comment.
@MichielCottaar I think that passing the voxel/vertex/structure elements separated as in the attributes presented on line 130 might be an easier to understand interface. The typed arrays with -1 harcoded for an ignored value seems suboptimal as interface design. @effigies , you have more the hand on what's the interface style in nibabel, what do you think?
Alternatively, this constructor could be hidden and just creating the objects from the factory methods, such as from_mask, which have very comfy and clear interfaces, might be a good option.
Codecov Report
@@ Coverage Diff @@
## master #641 +/- ##
==========================================
- Coverage 89.57% 88.11% -1.47%
==========================================
Files 93 188 +95
Lines 11470 23998 +12528
Branches 1991 4258 +2267
==========================================
+ Hits 10274 21145 +10871
- Misses 856 2158 +1302
- Partials 340 695 +355
Continue to review full report at Codecov.
|
|
I just rebased onto master and adjusted the documentation according to your first suggestion. I agree that the typed array is not the most natural interface. The main reason for doing this was that I was able to move some of the logic to the Axis class. However, in practice I ended up overriding most of these methods anyway. So perhaps I should just use the Axis class as a convenient class for type checking and to define the interface. I could even make it an abstract base class (https://docs.python.org/3.7/library/abc.html) to make this explicit. |
|
Great reactivity! Agreed that if finally you don't use anything of the Axis class, the ABC scenario is good design.
|
|
I have removed the underlying typed arrays as I agree that they were mainly confusing. The constructors are now more useful. Next step is to check why the style check in travis is failing. |
|
@MichielCottaar Might be worth merging master, to make sure it's not something unrelated that we fixed earlier. |
distinguishes between io using the raw header or using the new Cifti2 axes
Also added a few tests to increase coverage
- removed last reference of `arr` (replace with `time`) - removed spurious print statements in test
Removed Scalar.to_label, because it was no longer needed
Visually checked that the compiled documentation looks okay (not great)
There was a problem hiding this comment.
Hi @MichielCottaar, here's a first pass. I stopped at BrainModel because this is starting to get a bit long. I figured we could establish strategy, you could have another pass, and then I can finish the review. Let me know if that works for you.
I tried to limit overly nit-picky suggestions, but you should definitely feel free to argue against any of them.
nibabel/cifti2/cifti2_axes.py
Outdated
| other.volume_shape != shape | ||
| ): | ||
| raise ValueError("Trying to concatenate two BrainModels defined " + | ||
| "in a different brain volume") |
There was a problem hiding this comment.
Can simplify these 10 lines:
affine, shape = other.affine, other.volume_shape
if self.affine is not None and (
not np.allclose(affine, self.affine) or
self.volume_shape != shape):
raise ValueError(...)There was a problem hiding this comment.
If the other BrainModel has an affine or volume_shape of None (which is expected if it does not contain any voxels), this change will lead to an error.
The existing code ensures two things:
- If only one of the BrainModels has volumetric data (i.e. the affine is not set to None), the local variables affine and volume_shape will be set to that BrainModel.
- If both BrainModels have volumetric data, they are checked for consistency of the affine and volume_shape.
There was a problem hiding this comment.
This looks great! I made a few more comments, mostly in the docstrings.
The :class: linking I suggested may be a little overdone, so feel free not to go too wild in linking everything. I think in general it's good to link to objects that are not being directly documented, so hopefully that's what I suggested.
nibabel/cifti2/cifti2_axes.py
Outdated
|
|
||
| >>> from nibabel import cifti2 | ||
| >>> bm_cortex = cifti2.BrainModel.from_mask(cortex_mask, | ||
| ... brain_structure='cortex_left') # doctest: +SKIP |
There was a problem hiding this comment.
Are you able to generate these (or dummy) masks easily? Since we run doctests on docs like this, it would help to ensure that the docs are providing usable suggestions. If not, we can add this to the list of things to look into when we do a big sweep through the docs.
There was a problem hiding this comment.
I created some small, fake masks. I guess a better example would load some atlas and select a single ROI to include in the BrainModelAxis, but I am not aware of one easily availble
Co-Authored-By: MichielCottaar <[email protected]>
Co-Authored-By: MichielCottaar <[email protected]>
DOC: Mostly documentation updates
|
Any ideas about the appveyor errors, @effigies ? They only happened in python 3.4 for the 32-bit system and seem unrelated with the changes I made in the code. |
|
@MichielCottaar Those errors are stochastic but persistent. I'm currently not worrying about them. Thanks for this. @demianw Any final comments? |
|
Nope! Great job @MichielCottaar! |
|
Thanks for your patience. |
|
Thanks for all your great feedback. |
|
Yay!!!!! |
|
awesome job @MichielCottaar . this will help us move things forward. i asked @effigies if we can start doing some interactive tutorials for nibabel that would serve as both static tutorials but also something people can immediately start playing with online. we can use the nipype tutorials as an example: https://miykael.github.io/nipype_tutorial/ |
For consideration: * PySurfer : 13 (Alex Gramfort contributed FreeSurfer IO initially used there IIRC) * MichielCottaar/cifti : 60 (nipy/nibabel#641 deprecated that tool altogether)
This pull request is designed to add semantic understanding of the Cifti2 file format to nibabel. This is essentially a port from https://github.com/MichielCottaar/cifti as recently suggested to me by @demianw .
In short, the Cifti2 file format allows for the storage off matrices (or tensors) with a header describing what each row/column means. This meaning can be assigning to every row/column either a voxel or vertex (BrainModel), a group of voxels and/or vertices (Parcel), simply assigning a name (Scalar), a name and a color map (Label), or a starting point and step (Series). All these different options are stored in the same XML structure and hence have the same class representation in the current Cifti2Header file.
This pull request defines a new cifti2_axes.Axis class, which is a superclass of all the options defined above (BrainModel, Parcel, Scalar, Label, and Series). These new class all support:
There is quite a lot going on here. I would welcome any feedback on the current feature set, the API, and whether people think it is a good idea to include this in nibabel at all.