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

Updates to mesh vertices and centroids #2711

Merged
merged 8 commits into from Oct 17, 2023

Conversation

pshriwise
Copy link
Contributor

Description

This PR adjusts the vertices and centroid properties on the Python API mesh classes s.t. they always return Cartesian coordinates for any mesh type. Methods for getting vertices and centroid coordinates in other systems have been added for CylindricalMesh and SphericalMesh.

I've also updated the UnstructuredMesh vertices, centroids, and connectivity properties to return the same structure as the other classes with a shape (3, ...) so that they can be unpacked as x, y, z = mesh.centroids when needed.

Checklist

  • I have performed a self-review of my own code
    - [ ] I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
    - [ ] I have added tests that prove my fix is effective or that my feature works (if applicable)

@pshriwise
Copy link
Contributor Author

I think we hashed a lot of this out offline @eepeterson. In addition to discussing #2720 and #2721, we settled on keeping vertices and centroids as properties with properties for local coordinate systems where applicable (cylindrical and spherical mesh). In the future, we may add a more generic method like get_vertices that would allow an arbitrary transformation to be provided.

FWIW, this is similar to the approach for the Tally.mean property and Tally.get_values method.

Copy link
Contributor

@eepeterson eepeterson left a comment

Choose a reason for hiding this comment

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

LGTM!

@eepeterson eepeterson merged commit 4d3a198 into openmc-dev:develop Oct 17, 2023
18 checks passed
@shimwell
Copy link
Member

Looking forward to making use of this new feature

Would there be any benefit in renaming

cylindrical_vertices to vertices _cylindrical
cylindrical_centroids to centroids_cylindrical
Spherical_vertices to vertices_spherical
Spherical_centroids to centroids_spherical

Then it matches the first word of the strucutred mesh methods and might be more findable with tab complete

Sorry I'm late to comment

@pshriwise
Copy link
Contributor Author

Then it matches the first word of the strucutred mesh methods and might be more findable with tab complete

Good point! I'll make that update. Thanks @shimwell!

stchaker pushed a commit to stchaker/openmc that referenced this pull request Oct 25, 2023
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.

None yet

3 participants