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

SurfaceTransform class #203

Merged
merged 35 commits into from
Jun 22, 2024
Merged

SurfaceTransform class #203

merged 35 commits into from
Jun 22, 2024

Conversation

feilong
Copy link
Contributor

@feilong feilong commented May 16, 2024

Transforms data between different surface spaces.

Dylan is working on algorithms to derive the transforms.


Parameters
----------
x : array-like, shape (..., nv1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is nv1? number of vertices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Parameters
----------
x : array-like, shape (..., nv1)
Data to transform.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is "data" in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking typically either functional data (2D data matrices, e.g., response time series) or anatomical data (1D maps, e.g., thickness).

return filename

@classmethod
def from_filename(cls, filename, fmt=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

from filename would not read sphere.reg and so forth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

codecov bot commented May 20, 2024

Codecov Report

Attention: Patch coverage is 89.92629% with 41 lines in your changes missing coverage. Please review.

Project coverage is 94.39%. Comparing base (76832f5) to head (f69faaf).
Report is 89 commits behind head on master.

Files with missing lines Patch % Lines
nitransforms/surface.py 89.37% 26 Missing and 13 partials ⚠️
nitransforms/base.py 94.87% 1 Missing and 1 partial ⚠️
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     
Flag Coverage Δ
unittests 94.39% <89.92%> (-1.40%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shotgunosine
Copy link
Contributor

Shotgunosine commented May 21, 2024

From the documentation for wb_command -surface-sphere-project-unproject:

A surface registration starts with an input sphere, and moves
its vertices around on the sphere until it matches the template data.
This means that the registration deformation is actually represented as
the difference between two separate files - the starting sphere, and the
registered sphere. Since the starting sphere of the registration may not
have vertex correspondence to any other sphere (often, it is a native
sphere), it can be inconvenient to manipulate or compare these
deformations across subjects, etc.

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:
Ok actually the Surface Coordinate Transform class will already represent this deformation.

@oesteban
Copy link
Collaborator

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 ;)

@Shotgunosine
Copy link
Contributor

@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.

@oesteban
Copy link
Collaborator

@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.

@Shotgunosine
Copy link
Contributor

In terms of use cases for surface transformations, here's what I can think of:

  1. Taking some metric defined on the cortical surface in subject T1w space and determining what the value of that metric would be for every vertex in a reference space (wb_command -metric-resample).
  2. Taking labels defined in a reference space and determining which vertices in subject space those labels should be applied to (wb_command -label-resample).
  3. Creating a surface in which every vertex corresponds to a vertex from a reference surface, but with coordinates defined in the subject's T1w space (wb_command -surface-sphere-project-unproject).

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 surface-sphere-project-unproject works, but I feel like that's the next use case to try implementing to confirm that this representation of surface transformation is sufficient.

@feilong
Copy link
Contributor Author

feilong commented Jun 4, 2024

Hi @Shotgunosine, I think surface-sphere-project-unproject is closely related to the transform @oesteban wants.
It defines the warp using two spheres (e.g., sphere and sphere.reg). If a new sphere X is aligned to one of them (say, sphere.reg), we can use the barycentric weights (between sphere.reg and X) and the coordinates of sphere to warp X. That is, computing the new coordinates of X when it's aligned to sphere space. Conceptually, it's a diffeomorphic transform defined by the two spheres.
In practice, this is hardly used. The only use case I know is warping between fsaverage and fs_LR spheres. That being said, it's good to have it for completeness.

Comment on lines 289 to 292
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)
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused why this is decomposed. Is it a mathematical requirement, or a hack to get things working, or...?

Dockerfile Outdated Show resolved Hide resolved
Shotgunosine and others added 2 commits June 21, 2024 09:05
Co-authored-by: Oscar Esteban <code@oscaresteban.es>
Comment on lines 85 to 88
reference: surface
Surface with the destination coordinates for each index.
moving: surface
Surface with the starting coordinates for each index.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this is defined in reverse.

Suggested change
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.

Comment on lines 97 to 102
if inverse:
source = self.reference
dest = self.moving
else:
source = self.moving
dest = self.reference
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reference gives typically the sampling, so like above:

Suggested change
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

Comment on lines +133 to +137
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this matches my understanding of reference and moving

Copy link
Collaborator

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks for putting this together.

We still disagree on what reference and moving is, but I'm happy to go ahead and swap them the day I manage to convince you of the convention :D

.gitignore Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me get this in in a separate PR

Dockerfile Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me get this in in a separate PR

nitransforms/surface.py Outdated Show resolved Hide resolved
nitransforms/tests/test_surface.py Outdated Show resolved Hide resolved
Comment on lines 101 to 102
assert scti + sct == SurfaceCoordinateTransform(pial, pial)
assert sct + scti == SurfaceCoordinateTransform(sphere_reg, sphere_reg)
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
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)

Comment on lines 104 to 105
sct.reference = pial
sct.moving = sphere_reg
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
sct.reference = pial
sct.moving = sphere_reg
sct.reference = sphere_reg
sct.moving = pial

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not used?

nitransforms/tests/test_surface.py Outdated Show resolved Hide resolved
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This brings the cortical thickness from the subject into the fslr sphere, correct?

nitransforms/surface.py Outdated Show resolved Hide resolved
nitransforms/base.py Outdated Show resolved Hide resolved
@oesteban oesteban merged commit d5c8a25 into nipy:master Jun 22, 2024
8 checks passed
@@ -27,6 +27,7 @@ install_requires =
scipy >= 1.6.0
nibabel >= 3.0
h5py
pathlib
Copy link
Member

@shnizzedy shnizzedy Sep 6, 2024

Choose a reason for hiding this comment

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

I know this has been merged in for a while, but why include pathlib in install_requires when

python_requires = >= 3.8
and pathlib was incorporated into the standard Python library in v3.4?

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.0

which requires

nitransforms>=21.0.0

)

shnizzedy added a commit to FCP-INDI/cpac that referenced this pull request Sep 6, 2024
shnizzedy added a commit to FCP-INDI/cpac that referenced this pull request Sep 6, 2024
@effigies
Copy link
Member

effigies commented Sep 6, 2024

@shnizzedy I agree, this should be removed. Could you open a PR?

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