SurfaceTransform class #203
Conversation
nitransforms/surface.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| x : array-like, shape (..., nv1) |
There was a problem hiding this comment.
what is nv1? number of vertices?
There was a problem hiding this comment.
Yes, the number of vertices. I use nv1 for the number of vertices of input, and nv2 for the output, given they are likely to be different.
nitransforms/surface.py
Outdated
| Parameters | ||
| ---------- | ||
| x : array-like, shape (..., nv1) | ||
| Data to transform. |
There was a problem hiding this comment.
What is "data" in this context?
There was a problem hiding this comment.
I was thinking typically either functional data (2D data matrices, e.g., response time series) or anatomical data (1D maps, e.g., thickness).
nitransforms/surface.py
Outdated
| return filename | ||
|
|
||
| @classmethod | ||
| def from_filename(cls, filename, fmt=None): |
There was a problem hiding this comment.
from filename would not read sphere.reg and so forth?
There was a problem hiding this comment.
This one is only reading the sparse matrix from a file.
With the code Dylan's working on, we can generate the sparse matrix from the spheres.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #203 +/- ##
==========================================
- Coverage 95.79% 94.39% -1.40%
==========================================
Files 14 15 +1
Lines 1307 1713 +406
Branches 259 323 +64
==========================================
+ Hits 1252 1617 +365
- Misses 52 79 +27
- Partials 3 17 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
From the documentation for
Based on that description, I think the next class we need is one that will represent registration deformation and contains the original sphere and the deformation sphere (in other words, mea culpa, @oesteban was right). I'm not 100% certain it will be useful for anything (maybe project-unproject?), but it is the representation of the deformation. Edit: |
|
As I mentioned, I totally see why you wanted to focus on the interpolation matrix. In practical terms, it's all you care for. But to calculate it, in one way or another you need to formalize the transform ;) |
|
@oesteban Happy to talk about it more, but I think the SurfaceCoordinateTransform is the formalization of the transform. Isn't it? The deformation itself isn't useful and transforming data by subtracting the coordinate would less accurate than looking up the new coordinates by index. |
Yup, that's correct. |
|
In terms of use cases for surface transformations, here's what I can think of:
We can kinda do 1 and 2 with the resampling method we've got now (though we've only implemented barycentric for this and they recommend adap_bary_area). I still have trouble wrapping my head around how |
|
Hi @Shotgunosine, I think |
nitransforms/surface.py
Outdated
| rs_x = x._coords[:, 0] @ mat | ||
| rs_y = x._coords[:, 1] @ mat | ||
| rs_z = x._coords[:, 2] @ mat | ||
| y = SurfaceMesh.from_arrays(np.vstack([rs_x, rs_y, rs_z]).T, self.reference._triangles) |
There was a problem hiding this comment.
I'm confused why this is decomposed. Is it a mathematical requirement, or a hack to get things working, or...?
Co-authored-by: Oscar Esteban <code@oscaresteban.es>
nitransforms/surface.py
Outdated
| reference: surface | ||
| Surface with the destination coordinates for each index. | ||
| moving: surface | ||
| Surface with the starting coordinates for each index. |
There was a problem hiding this comment.
I feel this is defined in reverse.
| reference: surface | |
| Surface with the destination coordinates for each index. | |
| moving: surface | |
| Surface with the starting coordinates for each index. | |
| reference: surface | |
| Surface with the starting coordinates for each index. | |
| moving: surface | |
| Surface with the destination coordinates for each index. |
nitransforms/surface.py
Outdated
| if inverse: | ||
| source = self.reference | ||
| dest = self.moving | ||
| else: | ||
| source = self.moving | ||
| dest = self.reference |
There was a problem hiding this comment.
Reference gives typically the sampling, so like above:
| if inverse: | |
| source = self.reference | |
| dest = self.moving | |
| else: | |
| source = self.moving | |
| dest = self.reference | |
| if not inverse: | |
| source = self.reference | |
| dest = self.moving | |
| else: | |
| source = self.moving | |
| dest = self.reference |
| reference: spherical surface of the reference space. | ||
| Output will have number of indices equal to the number of indicies in this surface. | ||
| Both reference and moving should be in the same coordinate space. | ||
| moving: spherical surface that will be resampled. | ||
| Both reference and moving should be in the same coordinate space. |
There was a problem hiding this comment.
this matches my understanding of reference and moving
.gitignore
Outdated
There was a problem hiding this comment.
Let me get this in in a separate PR
Dockerfile
Outdated
There was a problem hiding this comment.
Let me get this in in a separate PR
nitransforms/tests/test_surface.py
Outdated
| assert scti + sct == SurfaceCoordinateTransform(pial, pial) | ||
| assert sct + scti == SurfaceCoordinateTransform(sphere_reg, sphere_reg) |
There was a problem hiding this comment.
| assert scti + sct == SurfaceCoordinateTransform(pial, pial) | |
| assert sct + scti == SurfaceCoordinateTransform(sphere_reg, sphere_reg) | |
| assert sct + scti == SurfaceCoordinateTransform(pial, pial) | |
| assert scti + sct == SurfaceCoordinateTransform(sphere_reg, sphere_reg) |
nitransforms/tests/test_surface.py
Outdated
| sct.reference = pial | ||
| sct.moving = sphere_reg |
There was a problem hiding this comment.
| sct.reference = pial | |
| sct.moving = sphere_reg | |
| sct.reference = sphere_reg | |
| sct.moving = pial |
nitransforms/tests/test_surface.py
Outdated
| def test_SurfaceCoordinateTransformIO(testdata_path, tmpdir): | ||
| sphere_reg_path = testdata_path / "sub-sid000005_ses-budapest_acq-MPRAGE_hemi-R_space-fsLR_desc-reg_sphere.surf.gii" | ||
| pial_path = testdata_path / "sub-sid000005_ses-budapest_acq-MPRAGE_hemi-R_pial.surf.gii" | ||
| fslr_sphere_path = testdata_path / "tpl-fsLR_hemi-R_den-32k_sphere.surf.gii" |
| pial_path = testdata_path / "sub-sid000005_ses-budapest_acq-MPRAGE_hemi-R_pial.surf.gii" | ||
|
|
||
| # test project-unproject funcitonality | ||
| projunproj = SurfaceResampler(sphere_reg_path, fslr_sphere_path) |
There was a problem hiding this comment.
I agree with reference and moving here
| moving = sphere_reg | ||
| # compare results to what connectome workbench produces | ||
| resampling = SurfaceResampler(reference, moving) | ||
| resampled_thickness = resampling.apply(subj_thickness.agg_data(), normalize='element') |
There was a problem hiding this comment.
This brings the cortical thickness from the subject into the fslr sphere, correct?
Co-authored-by: Oscar Esteban <code@oscaresteban.es>
Co-authored-by: Oscar Esteban <code@oscaresteban.es>
Co-authored-by: Oscar Esteban <code@oscaresteban.es>
| @@ -27,6 +27,7 @@ install_requires = | |||
| scipy >= 1.6.0 | |||
| nibabel >= 3.0 | |||
| h5py | |||
| pathlib | |||
There was a problem hiding this comment.
I know this has been merged in for a while, but why include pathlib in install_requires when
Line 24 in f69faaf
This requirement overrides the current-Python-version's pathlib with the version on PyPI for Python 3.3, which breaks pip (at least in C-PAC's current release Dockerfile, which includes
sdcflows==2.4.0which requires
nitransforms>=21.0.0)
|
@shnizzedy I agree, this should be removed. Could you open a PR? |
Transforms data between different surface spaces.
Dylan is working on algorithms to derive the transforms.