Adds parametric type system for PhysicsNeMo-Mesh#1500
Adds parametric type system for PhysicsNeMo-Mesh#1500peterdsharpe merged 9 commits intoNVIDIA:mainfrom
Conversation
- Introduced `MeshDims` class for dimension-aware type hints in the `Mesh` class. - Implemented `__class_getitem__` method in `Mesh` to support subscript syntax for specifying manifold and spatial dimensions. - Added convenience aliases: `PointCloud` for 0-manifold and `Graph` for 1-manifold. - Updated mesh representation formatting to include dimension information. - Added comprehensive tests for the new functionality, including validation of dimension constraints and type checks.
- Updated the conditional check for string types in the MeshDims class to enhance code clarity by formatting the line breaks.
- Removed string type assertions for manifold and spatial dimensions in the MeshDims class. - Updated dimension parsing to include type ignore comments for better compatibility with type checkers.
Greptile SummaryThis PR adds a parametric type system for Key items to be aware of:
Important Files Changed
Last reviewed commit: 32c1b63 |
coreyjadams
left a comment
There was a problem hiding this comment.
I don't have any particular objection to this though I kind of liked the old way better. It was more explicit: you are used to writing this every day and remember if the manifold dimension or spatial dimension is first but the rest of us don't. I liked using the keywords.
Is this just for representations? Or is this driving type checking and match cases too?
- Improved error handling for symbolic dimensions in the MeshDims class by adding checks to ensure paired dimensions are provided. - Updated parsing logic to cache results for symbolic dimensions, enhancing performance and clarity. - Added new tests to validate the behavior of symbolic dimensions and ensure proper error messages are raised when constraints are not met.
|
/blossom-ci |
|
Thanks for the speedy review @coreyjadams ! :) Re:
So, the nice thing here is that it's actually unambiguous, since
This ordering also matches how mathematicians discuss these: "Let M be a 2d manifold in 3d space" -> "Mesh[2, 3]". My feeling is that the conciseness (at least in terms of type signatures) is a nice win here, especially as we start doing more complicated Mesh ops (e.g., bundling a surface mesh and a volume mesh together, so we can manipulate them as one). That said, I can see how having it be more explicit in the |
…on information - Modified the string representation of the Mesh class to specify manifold and spatial dimensions explicitly. - Updated related test cases to reflect the new format in expected outputs, ensuring consistency across representations.
…n representation - Modified the assertion in the test case to check for the updated string format that includes explicit manifold and spatial dimensions. - Ensured consistency with recent changes in the Mesh class representation.
|
/blossom-ci |
|
/blossom-ci |
1 similar comment
|
/blossom-ci |
Parametric type hints for Mesh
Adds subscript syntax to
Meshso thatMesh[2, 3]denotes a 2-dimensional manifold embedded in 3-dimensional space. This enables dimension-aware type annotations and runtimeisinstancechecks with no changes to Mesh's data model or serialization.Subscript syntax
Mesh[m, s]always takes exactly two parameters. Use...(Ellipsis) for an unconstrained dimension. Concrete integer dimensions are validated at spec-creation time (non-negative, manifold <= spatial). Symbolic string labels like"n-1"are parsed and, when both dimensions share a variable, the implied codimension is validated atisinstancetime.Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.