feat(evaluation): detect part topology inconsistencies - #5
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3392753797
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| from scipy import ndimage | ||
| from skimage.draw import polygon | ||
| except Exception: | ||
| return -1 |
There was a problem hiding this comment.
If scipy or skimage is unavailable, mesh_projected_holes returns -1 silently, and callers treat that as a normal numeric result. In environments where trimesh is installed but those optional deps are not, the new topology diagnostics are effectively disabled without any warning, so reports can look healthy while the projected-hole consistency checks never ran.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bcbdfae (bcbdfaed7f69e8cb85bc9946da7aef1cceec8549).
What changed:
mesh_projected_holes()no longer silently returns-1whenscipy/skimageimports fail.check_part_topology()catches the missing dependency once, emits a warning (projected-hole topology checks skipped; missing dependency: ...), and recordsNonefor projected-hole counts instead of treating a sentinel as normal data.
Verification:
conda run -n physx-decoder python -m py_compile scripts/inspect_pipeline_result.py- Local scissors inspection still reports the repeated
Blade Handle Setprojected-hole mismatch. - Monkeypatched import test verifies missing
scipy/skimageemits the skipped-check warning.
Extend result inspection with projected hole counts, semantic part names, and repeated-part consistency checks. This catches cases like scissors where one blade-handle counterpart preserves an opening while the other is filled, and it reports when optional projected-hole dependencies are unavailable. Constraint: Mesh quality issues need deterministic local warnings before adding human review or VLM judging. Rejected: Treat every handle name as requiring a hole | microwave handles are solid protrusions, so repeated-part inconsistency is a better first signal. Rejected: Silently return sentinel values when scipy/skimage are missing | skipped topology checks must be visible in the report. Confidence: high Scope-risk: narrow Directive: Keep topology warnings advisory and avoid hard failures unless meshes are empty or unreadable. Tested: conda run -n physx-decoder python -m py_compile scripts/inspect_pipeline_result.py; inspected local scissors result and got repeated Blade Handle Set projected-hole mismatch warning; inspected local microwave result without false handle-hole warning; monkeypatched import test verifies missing scipy/skimage emits a projected-hole skipped warning. Not-tested: Robustness across arbitrary camera orientations and non-manifold meshes.
3392753 to
bcbdfae
Compare
|
@codex review Review requested after addressing topology dependency feedback:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcbdfae69e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mapping = {} | ||
| for part in basic_info.get("parts", []): | ||
| if isinstance(part, dict) and "label" in part: | ||
| mapping[str(part.get("label"))] = str(part.get("name", "")) |
There was a problem hiding this comment.
Converting part.get("name", "") with str(...) turns JSON null names into the literal string 'None', so unnamed parts are treated as a real semantic name and can be grouped as repeated 'none' parts, producing misleading topology inconsistency warnings. This is triggered whenever basic_info.json includes "name": null; using an empty-string fallback like part.get("name") or "" avoids false positives.
Useful? React with 👍 / 👎.
1 participant
Summary
inspect_pipeline_result.pywith part topology diagnosticsbasic_info.jsonhandle-must-have-hole ruleVerification
conda run -n physx-decoder python -m py_compile scripts/inspect_pipeline_result.pyBlade Handle Setprojected-hole mismatchPASS_WITH_WARNINGSwithout a false handle-hole warningNotes
evaluation-plan).