Fix order-match-crop pipeline review findings - #12
Open
nathan-cain15 wants to merge 2 commits into
Open
Conversation
nathan-cain15
commented
Aug 26, 2026
nathan-cain15
commented
- merge_annotations.py: correct parse_cvat_xml return type/docstring to reflect the (annotations, dimensions) tuple; make dry-run simulation mirror the real merge path's skip-on-unexpected-trayID behavior for -and- images
- crop_and_link.py: use individual_ids to write a sidecar {stem}_ids.csv linking each crop's position/filename to its individualID; return actual saved-crop count instead of the ordered box count
- run_pipeline.sh: clean up temp SLURM job scripts on exit; fix output description to match actual {tray}_{N}.png + sidecar CSV naming
- README.md: update docs to match the sidecar-CSV linking behavior
- merge_annotations.py: correct parse_cvat_xml return type/docstring
to reflect the (annotations, dimensions) tuple; make dry-run
simulation mirror the real merge path's skip-on-unexpected-trayID
behavior for -and- images
- crop_and_link.py: use individual_ids to write a sidecar
{stem}_ids.csv linking each crop's position/filename to its
individualID; return actual saved-crop count instead of the
ordered box count
- run_pipeline.sh: clean up temp SLURM job scripts on exit; fix
output description to match actual {tray}_{N}.png + sidecar CSV
naming
- README.md: update docs to match the sidecar-CSV linking behavior
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses review findings in the “order-match-crop” pipeline by aligning behavior and documentation around CVAT merge semantics and crop-to-individualID linkage, while improving operational hygiene for SLURM job-script temp files.
Changes:
- Updates CVAT XML parsing/typing and makes dry-run simulation match real merge “skip on unexpected trayID” behavior for
-and-images. - Adds sidecar
{stem}_ids.csvgeneration to map crop position/filename →individualID, and returns the actual number of crops saved. - Cleans up temporary SLURM job scripts on exit and updates pipeline docs/messages to reflect the sidecar mapping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| order_match_crop_for_order_finalized/run_pipeline.sh | Adds temp-script cleanup on exit and updates user-facing output descriptions. |
| order_match_crop_for_order_finalized/README.md | Updates pipeline documentation to describe sidecar _ids.csv linkage behavior. |
| order_match_crop_for_order_finalized/merge_annotations.py | Corrects parse_cvat_xml return type/docstring and aligns dry-run simulation with real merge behavior for unexpected trayIDs. |
| order_match_crop_for_order_finalized/crop_and_link.py | Writes {stem}_ids.csv sidecar mapping and returns actual saved-crop count; updates count mismatch messaging. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+31
to
+33
| TMP_FILES=() | ||
| cleanup() { rm -f "${TMP_FILES[@]}"; } | ||
| trap cleanup EXIT |
Comment on lines
+40
to
+44
| def parse_cvat_xml( | ||
| xml_path: Path, | ||
| ) -> tuple[dict[str, list[tuple[float, float, float, float]]], dict[str, tuple[int, int]]]: | ||
| """ | ||
| Return a dict mapping image name → list of (xtl, ytl, xbr, ybr) float tuples. | ||
| Return a 2-tuple of: |
Comment on lines
+297
to
+302
| if id_rows: | ||
| ids_csv = output_dir / f"{file_stem}_ids.csv" | ||
| with open(ids_csv, "w", newline="") as fh: | ||
| writer = csv.writer(fh) | ||
| writer.writerow(["position", "filename", "individualID"]) | ||
| writer.writerows(id_rows) |
| echo "" | ||
| echo "Output subfolders:" | ||
| echo " ${OUTPUT}/cropped/ — beetle crops named {individualID}.png" | ||
| echo " ${OUTPUT}/cropped/ — beetle crops named {tray}_{N}.png, plus a {tray}_ids.csv linking each crop to its individualID" |
| ``` | ||
| Cropped/ | ||
| ├── cropped/ # beetle crops named {tray}_{N}.png, in spatial order | ||
| ├── cropped/ # beetle crops named {tray}_{N}.png, in spatial order, plus {tray}_ids.csv linking each crop to its individualID |
- run_pipeline.sh: guard EXIT cleanup trap so an empty TMP_FILES
array doesn't call rm with no operands; clarify that cropped/
output uses per-tray subfolders, not flat files
- merge_annotations.py: tighten parse_cvat_xml's local variable
annotations to match its declared precise return types
- crop_and_link.py: only write the {stem}_ids.csv sidecar when every
ordered box was actually saved (none skipped as out-of-bounds), so
a partial crop set never produces a misleadingly-partial mapping
- README.md: fix output tree to reflect per-tray subfolders
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.