Mesh: honor force_copy throughout PyVista conversion - #1785
Conversation
|
/ok to test a1dd036 |
|
/ok to test e039fd5 |
There was a problem hiding this comment.
Pull request overview
This PR strengthens copy-ownership guarantees in the PyVista ↔ physicsnemo.mesh.Mesh conversion helpers by ensuring force_copy=True covers attached data arrays (point/cell/global) and by adding a matching force_copy option on export via to_pyvista.
Changes:
- Extend
from_pyvista(..., force_copy=True)to deep-copy point/cell/field data containers (including centroid mode). - Add keyword-only
to_pyvista(..., force_copy=True)to prevent exported PyVista objects from aliasing the sourceMesh’s CPU storage. - Add mutation-based regression tests and update the changelog entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
physicsnemo/mesh/io/io_pyvista.py |
Applies force_copy to VTK attribute conversion and adds to_pyvista(..., force_copy=...) copying semantics. |
test/mesh/io/io_pyvista/test_data_preservation.py |
Adds regression tests asserting force_copy prevents cross-object mutation for geometry and attached data. |
CHANGELOG.md |
Documents the strengthened force_copy behavior and new to_pyvista option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR fixes incomplete ownership isolation in the PyVista ↔
Important Files Changed
Reviews (1): Last reviewed commit: "Merge branch 'main' into codex/mesh-pyvi..." | Re-trigger Greptile |
laserkelvin
left a comment
There was a problem hiding this comment.
I'll conditionally approve - the thing I'm mostly thinking about is this is really only a problem when doing compute on host right? Would we consider this undesirable anyway, and users should be moving the data to device?
|
/ok to test 7e00bbe |
|
/ok to test e88afd7 |
PhysicsNeMo Pull Request
Description
from_pyvista(..., force_copy=True)previously copied geometry but still allowed attached point, cell, and global arrays to share storage with the source PyVista object. Mutating the returnedMeshcould therefore mutate an input whose caller requested independent ownership. The reverse conversion had no matching opt-in ownership guarantee.This PR:
force_copyto imported point, cell, and global data, including cell-centroid mode.to_pyvista(..., force_copy=True)for independent exported geometry and attached data.Checklist
Dependencies
maininto this branch and retain its precision-preserving geometry conversion while applying the copy-ownership behavior here.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.