Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

ENH: Add TriangularMesh data structure [BIAP9] #1257

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

Open
effigies wants to merge 8 commits into nipy:master
base: master
Choose a base branch
Loading
from effigies:enh/triangular_mesh

Conversation

Copy link
Member

@effigies effigies commented Sep 19, 2023

#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

Copy link

codecov bot commented Sep 19, 2023
edited
Loading

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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1 participant

AltStyle によって変換されたページ (->オリジナル) /