-
Notifications
You must be signed in to change notification settings - Fork 473
Add explicit error message for no world edges #1183
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
Conversation
|
/blossom-ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR replaces graceful handling of empty world edges in two graph construction helpers with explicit error raising. Previously, when no world edges were found (when connecting mesh nodes to world/collider nodes based on a radius threshold), the functions returned the graph unchanged with empty world edge features. Now, both examples/structural_mechanics/deforming_plate/helpers.py and the DGL variant in deforming_plate_dgl/helpers.py raise a ValueError("No world edges to add") when world_edges.numel() == 0. The change also improves empty-tensor detection by using numel() instead of checking shape[1], making it more robust to malformed tensors. This converts what was treated as a valid edge case into a hard failure, ensuring that missing world edges—likely caused by incorrect world_edge_radius or missing world position data—are caught immediately rather than silently propagating through the simulation pipeline.
PR Description Notes:
- The PR description is empty beyond the template; no description, linked issue, or CHANGELOG update is provided
- All checklist items remain unchecked
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/deforming_plate/helpers.py | 2/5 | Replaces silent early-return for empty world edges with explicit ValueError; changes empty check from shape[1] to numel() |
| examples/structural_mechanics/deforming_plate_dgl/helpers.py | 2/5 | Identical behavioral change to PyG version: converts empty world edges from valid edge case to error condition |
Confidence score: 2/5
- This PR introduces a breaking behavioral change that may fail valid simulations where world edges legitimately don't exist due to geometry or radius parameters
- Score reflects that the change converts previously-working code into error conditions without clear justification, documentation, or tests verifying when empty world edges should be errors versus valid states
- Both
helpers.pyfiles need careful review to determine whether this error-on-empty behavior is correct for all simulation configurations, and whether downstream code assumes world edges can be absent
Sequence Diagram
sequenceDiagram
participant Caller
participant add_world_edges
participant load_json
participant radius_search
participant Graph
Caller->>add_world_edges: add_world_edges(graph, world_edge_radius, edge_stats_path)
add_world_edges->>Graph: clone()
Graph-->>add_world_edges: cloned graph
add_world_edges->>load_json: load_json(edge_stats_path)
load_json-->>add_world_edges: edge_stats (edge_mean, edge_std)
add_world_edges->>Graph: get existing mesh edges
Graph-->>add_world_edges: mesh_src, mesh_dst
add_world_edges->>Graph: get world_pos
Graph-->>add_world_edges: world_pos
add_world_edges->>radius_search: radius_search(world_pos, world_pos, radius)
radius_search-->>add_world_edges: edge_index
add_world_edges->>add_world_edges: filter self-loops and existing edges
alt No world edges found
add_world_edges->>Caller: raise ValueError("No world edges to add")
else World edges exist
add_world_edges->>add_world_edges: compute world edge features
add_world_edges->>add_world_edges: compute mesh edge features
add_world_edges->>Graph: add world edges and update edge attributes
add_world_edges->>Caller: return graph, mesh_edge_features, world_edge_features
end
2 files reviewed, 3 comments
|
/blossom-ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
PhysicsNeMo Pull Request
Description
Checklist
Dependencies