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

ENH: Add TriangularMesh data structure [BIAP9] #1257

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

Conversation

effigies
Copy link
Member

#1251 Added Pointset and Grid. #1090 also proposes TriangularMesh, but with the refactoring seen in #1251, that seemed like a bit of extra work for minimal benefit to the transformations use-case. This PR re-adds triangular meshes.

Overall, the approach of multiple names feels clunky and pushes the concerns of surface families into pointsets. #1251 removed that, and this PR extends this by keeping TriangularMesh focused on the base data structure and moves TriangularMeshFamily logic into a CoordinateFamilyMixin class that permits swapping out coordinates with a .with_name() method (open to bikeshedding). Hence, we would change:

left_surfaces.get_coords(name='pial')

to:

left_surfaces.with_name('pial').get_coords()

It also follows the lead of Grid and gets rid of the omni-__init__ in favor of targeted factory classmethods.


Notes from previous discussion:

  • @oesteban wrote: "As a note, if some time we support other meshes with more than three edges, then n_triangles should be better named n_faces."
    • This was part of early discussions of BIAP9. IIRC the universality of triangular meshes makes them worth special-casing for now. FreeSurfer has old formats that use non-triangular meshes, and both FreeSurfer and we convert them to triangular on load:
      if magic in (QUAD_MAGIC, NEW_QUAD_MAGIC): # Quad file
      nvert = _fread3(fobj)
      nquad = _fread3(fobj)
      (fmt, div) = ('>i2', 100.0) if magic == QUAD_MAGIC else ('>f4', 1.0)
      coords = np.fromfile(fobj, fmt, nvert * 3).astype(np.float64) / div
      coords = coords.reshape(-1, 3)
      quads = _fread3_many(fobj, nquad * 4)
      quads = quads.reshape(nquad, 4)
      #
      # Face splitting follows
      #
      faces = np.zeros((2 * nquad, 3), dtype=int)
      nface = 0
      for quad in quads:
      if (quad[0] % 2) == 0:
      faces[nface] = quad[0], quad[1], quad[3]
      nface += 1
      faces[nface] = quad[2], quad[3], quad[1]
      nface += 1
      else:
      faces[nface] = quad[0], quad[1], quad[2]
      nface += 1
      faces[nface] = quad[0], quad[2], quad[3]
      nface += 1

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (4e7ad07) 92.19% compared to head (368c145) 92.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1257      +/-   ##
==========================================
+ Coverage   92.19%   92.22%   +0.02%     
==========================================
  Files          99       99              
  Lines       12458    12496      +38     
  Branches     2560     2565       +5     
==========================================
+ Hits        11486    11524      +38     
  Misses        648      648              
  Partials      324      324              
Files Changed Coverage Δ
nibabel/pointset.py 99.16% <100.00%> (+0.38%) ⬆️

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

@effigies effigies marked this pull request as ready for review September 20, 2023 14:13
@effigies effigies changed the title ENH: Add TriangularMesh data structure ENH: Add TriangularMesh data structure [BIAP9] Sep 20, 2023
Comment on lines +219 to +220
def add_coordinates(self, name, coordinates):
self._coords[name] = coordinates
Copy link
Member Author

Choose a reason for hiding this comment

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

This is simple but perhaps a little clunky. We could also do:

Suggested change
def add_coordinates(self, name, coordinates):
self._coords[name] = coordinates
def add_coordinates(self, *args, **kwargs):
self._coords.update(dict(*args, **kwargs))

This would allow it to take any inputs that would generate a dict, which might be a bit more ergonomic. I'm not 100% on this, so I haven't made the change.

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.

1 participant