feat(evaluation): add core result inspection plan - #4
Conversation
Add the evaluation automation roadmap and a core result inspector that writes JSON and Markdown summaries for generated PhysX-Anything outputs. This establishes the report schema needed by topology checks, human review pages, render previews, and optional VLM evaluation. Constraint: Evaluation work should start with deterministic local checks before adding VLM-as-judge or browser UI layers. Rejected: Implement HTML review first | it needs a stable inspection summary schema to display. Rejected: Commit runtime pipeline outputs | reports are generated from ignored pipeline_runs directories. Confidence: high Scope-risk: narrow Directive: Keep later evaluation checks appending to inspection_summary.json rather than inventing parallel result formats. Tested: conda run -n physx-decoder python -m py_compile scripts/inspect_pipeline_result.py; conda run -n physx-decoder python scripts/inspect_pipeline_result.py --help; ran inspector on local scissors and microwave results, both producing PASS_WITH_WARNINGS without hard errors. Not-tested: MuJoCo load check because mujoco is not installed in the decoder environment.
![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: cb924563af
ℹ️ 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".
|
|
||
| allind = result_dir / "allind.npy" | ||
| if allind.exists(): | ||
| arr = np.load(allind) |
There was a problem hiding this comment.
check_voxel_arrays wraps np.load for each ind_*.npy, but allind.npy is loaded without a guard. If allind.npy is truncated/corrupt (a common failure mode for interrupted runs), np.load raises here and the script exits before inspection_summary.json / inspection_report.md are written, which defeats the purpose of diagnosing failed outputs. Please catch this exception and record it in report["errors"] like the per-part loads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 05381cd (05381cde3687bbf084a947454d20374888d698ae).
What changed:
- Wrapped
np.load(allind.npy)intry/except, matching the per-partind_*.npyhandling. - Corrupt/truncated
allind.npyis now recorded inreport["errors"]asfailed to load allind.npy: ...instead of aborting the inspector before reports are written.
Verification:
conda run -n physx-decoder python -m py_compile scripts/inspect_pipeline_result.py- Focused temp-dir test creates an invalid
allind.npyand confirms the inspector records a report error rather than raising.
| if "-" in token: | ||
| a, b = map(int, token.split("-")) | ||
| if a > b: | ||
| descending.append(token) |
There was a problem hiding this comment.
The coordinate-text check currently validates token syntax and descending ranges, but it never enforces numeric bounds, so tokens like 50000 or 10-60000 are accepted without warning. In this pipeline, coord tokens are expected to represent 32^3 voxel indices; out-of-range values are later clipped during decoding, so this omission hides malformed generation output that the inspector is supposed to surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 05381cd (05381cde3687bbf084a947454d20374888d698ae).
What changed:
- Added
MAX_VOXEL_INDEX = 32 ** 3 - 1and bounds validation for both single coordinate tokens and range tokens incoord_*.txt. - Tokens like
50000or10-60000are now surfaced as warnings:has out-of-range voxel tokens, so malformed VLM output is visible before downstream clipping can hide it.
Verification:
- Focused temp-dir test with
coord_0.txtcontaining10 10-60000 50000confirms the out-of-range warning is emitted.
Keep inspection reports diagnostic even when allind.npy is unreadable and surface out-of-range coordinate text tokens before downstream clipping hides malformed VLM output. Constraint: Inspection must diagnose interrupted or malformed runs instead of aborting before report generation. Rejected: Let np.load(allind.npy) raise | that prevents inspection_summary.json and inspection_report.md from being written for broken outputs. Confidence: high Scope-risk: narrow Directive: Treat generated text and array load failures as report errors rather than unhandled inspector exceptions. Tested: conda run -n physx-decoder python -m py_compile scripts/inspect_pipeline_result.py; focused temp-dir test detects out-of-range coord tokens and records corrupt allind.npy as an error. Not-tested: Full inspection rerun on every historical pipeline_runs directory after this focused fix.
|
@codex review Review requested after addressing the inspector robustness feedback:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05381cdba9
ℹ️ 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".
| allind = result_dir / "allind.npy" | ||
| if allind.exists(): |
There was a problem hiding this comment.
check_voxel_arrays only enters the allind.npy branch when the file exists, so a result directory with allind.npy deleted (or never produced) records no error/warning and can still end up as PASS/PASS_WITH_WARNINGS. Because this artifact is part of the core voxel outputs inspected by this script, silently skipping its absence hides a broken/incomplete run and can let downstream evaluation proceed on incomplete data.
Useful? React with 👍 / 👎.
| report["allind"] = {"shape": list(arr.shape), "voxels": int(arr.shape[0]) if arr.ndim else 0} | ||
| if arr.ndim != 2 or arr.shape[1] != 3: | ||
| add_error(report, f"allind.npy has invalid shape: {arr.shape}") | ||
| elif total_ind_voxels and arr.shape[0] != total_ind_voxels: | ||
| add_warning(report, f"allind voxel count {arr.shape[0]} differs from sum(ind_*.npy) {total_ind_voxels}") |
There was a problem hiding this comment.
After loading allind.npy, the code checks only shape and voxel-count consistency, but never verifies that coordinates stay within [0, 31] (or reports min/max like ind_*.npy). A corrupted allind.npy with valid shape/count but out-of-range values will therefore pass this check, which weakens the inspector’s ability to catch invalid voxel data.
Useful? React with 👍 / 👎.
1 participant
Summary
PLAN.mdwith the proposed evaluation PR sequencescripts/inspect_pipeline_result.pyfor core result inspectionbasic_info.json, coordinate text, voxel arrays, OBJ parts, GLB load, and optional MuJoCo XML loadinspection_summary.jsonandinspection_report.mdinto result directoriesVerification
conda run -n physx-decoder python -m py_compile scripts/inspect_pipeline_result.pyconda run -n physx-decoder python scripts/inspect_pipeline_result.py --helpPASS_WITH_WARNINGSPASS_WITH_WARNINGSNotes
skipped_missing_packagewhen mujoco is not installed.pipeline_runs/directories and are not committed.