feat: staff-finding pipeline improvements — line following, coord fix, stave QA boxes - #53
Open
kyrieb-ekat wants to merge 27 commits into
Open
feat: staff-finding pipeline improvements — line following, coord fix, stave QA boxes#53kyrieb-ekat wants to merge 27 commits into
kyrieb-ekat wants to merge 27 commits into
Conversation
📝 WalkthroughWalkthroughThe change adds staffline tracing experiments, shared evaluation tooling, grouping and interpolation logic, line-fitting refinements, training support, documentation, datasets, generated fixtures, and portability updates to scripts and tests. ChangesStaff-finding pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
fit_centerline.py
- Add x_page_offset / y_page_offset fields to FitResult (default 0.0)
so crop-local coords can be converted to page-absolute downstream
- Add line-following refinement triggered when initial Huber residual
mean > 1.0 * scale_unit (multi-line box signal):
* _trace_line() slides left-to-right within a 1.5h band, emitting
one median-y per window — naturally follows any curve shape
* Refit uses cubic polynomial (LINE_FOLLOW_POLY_DEGREE=3) when
≥ 8 trace points available, else falls back to quadratic; handles
both C-shaped page arches and S-shaped parchment waves
* Only accepted if refit residual improves over initial fit
* Flagged as line_following_applied:deg3/deg2 in FitResult.flags
- Diagnostic overlay now shows cyan scatter dots for trace points when
line-following was applied; legend distinguishes from fitted line
group_staves.py
- _y_at_fit_center: add fit.y_page_offset so gap analysis uses
page-absolute y-positions (fixes all-gaps-near-zero bug that put
every line in one giant stave)
- _save_grouping_diagnostic drawing loop: add x/y page offsets to
xs/ys before cv2.polylines (fixes lines-at-origin visualization bug)
- After polylines loop, draw a red bounding rectangle + Sn label
around each stave group for quick visual QA
run_page.py
- Populate fit_result.x_page_offset / y_page_offset from actual_box
(ulx, uly) immediately after each fit_centerline call
test_fit_centerline.py
- Fix sys.path (was hardcoded /home/claude)
- Add test_two_line_box_line_following: constructs a ComponentFilterResult
directly with two-line pixel data, verifies line_following_applied flag
and tight residual (< 3 px) after the refit
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…and current sauvola settings.
…nore os metadata files .__
Adds GentAnt1475 folio 17 (left and right crops) with YOLO inference outputs (raw + corrected) as the primary test page for the implicit neural and interpolation experiments documented in experiments/implicit_neural/NOTES.md. Moves the Ordo Virtutum page from image-sets/ root into its own image-sets/ordo_virt_000/ subdirectory, matching the layout convention used for gent/. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Introduces staff-finding/experiments/ — a family of standalone runner
scripts that plug in after YOLO detection and exercise different
curve-fitting approaches. Each runner produces identical JSOMR JSON and
stave-grouping diagnostic PNGs so results are directly comparable.
shared_utils.py: common helpers (YOLO parsing, ExperimentFitResult
bridge, run_grouping_and_save) shared by every runner.
Runners added:
- dp_tracing/ Dynamic-programming horizontal-emphasis tracer
- gp_centerlines/ Gaussian-process Matern fit on ink-pixel coords
- implicit_neural/ Test-time MLP on pixel brightness (best result
on Gent ms: mode=8, see implicit_neural/NOTES.md)
- periodicity/ Autocorrelation comb (limited on per-box crops)
- heatmap_regression/ Planned; NOTES.md stubs the design
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…terpolate_staves
group_staves.py additions
- Stage 7: gap-based missing-line synthesis with two triggers:
(A) in-stave gap fill — fires when consecutive detected lines in
the same stave have a y-gap in [cut_threshold, max_threshold],
inserts n_missing lines evenly spaced between them
(B) edge extrapolation — grid-anchored at the bottommost detected
line, fills empty slots; requires >= mode_n//2 lines detected
(safety against over-synthesis on sparse/split staves)
- Adaptive thresholds: cut_threshold (Otsu/median), min_threshold
(noise floor = 0.5 * scale_unit), max_threshold (mean of
inter-stave gaps) — all three shown on the gap distribution chart
- Territory boundaries: interpolated lines are constrained to each
stave's y-range; top/bottom staves cannot extrapolate outside
their own detected extent
- Stage 6c _check_stave_rhythm: classifies each stave as normal /
under_populated / over_populated by comparing detected intra-stave
gap count to mode_n-1; also reports gap_cv (spacing consistency)
- Rhythm gate in Stage 7: staves flagged under/over_populated are
skipped entirely — their detected assignments are preserved as-is
- Diagnostic chart: per-bar colouring by rhythm status (red = under,
purple = over), annotated labels showing observed/expected counts,
shaded noise and missing-line trigger zones
- StaveGroupingResult gains min_threshold_px, interpolation_max_gap_px,
rhythm_anomalies fields; summary print shows stave count + avg
interpolate_staves.py (new)
Extracted from group_staves Stage 7 to keep grouping and synthesis
concerns separate. Contains InterpolatedLine dataclass,
_compute_interpolation_max_gap, _interpolate_between, and the public
interpolate_missing_lines() API. group_staves.py re-exports
InterpolatedLine for backward-compatible imports.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
eval_page.py evaluates a single page run against ground-truth annotations, reporting per-stave line-count accuracy and flagging rhythm anomalies vs. expected stave structure. eval_batch.py runs eval_page across an image-set directory and aggregates results into a summary CSV, enabling cross-method and cross-manuscript comparison once all five experiment runners are complete. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
staff-finding/README.md gives an overview of the pipeline stages, experiment variants, and output formats for new contributors. .gitignore updated to cover Python cache dirs and experiment output folders that should not be tracked. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…le test fix Stage 1 (component_filter.py — already committed): loosen merge thresholds (y-distance 0.5→1.0h, x-gap 5→10h) and add companion-score-floor retention so high-scoring non-overlapping fragments are kept alongside the merge winner. Diagnostics updated with cyan coloring for companions in panels 3–5. Stage 2 (run_page.py): add centerline_page section to each JSOMR record with page-absolute x_start/x_end/y_values (crop-local + x_page_offset/y_page_offset). Old centerline key preserved for backwards compatibility. Tests: fix stale assertion in test_grouping_with_missing_line_and_interpolation — interpolation was implemented in 572c025 but the test still expected []. E2E data: - 28may_stave-fulldata/: Ordo Virtutum 8-page run (8.6MB, all files) - 29may/: Gent right/left multi-variant run (JSON/CSV/TXT only, no PNGs) - pitch_finding_sample/: Gent right, 86 lines, 17 staves — stafflines JSON, stave grouping PNGs, summary CSV, stave grouping report for pitch-finding collaborator Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Iterate on centerline fitting, staff grouping, interpolation, and component filtering logic across the scripts/ pipeline and the experimental fitters (DP tracing, GP centerlines, implicit neural, periodicity), plus add a new staffline detector training script.
Remove the superseded Gent left-crop inference outputs (regenerated into left-inference/, kept local rather than tracked) and ignore the new addtl-gt/ ground-truth dataset, which is too large to track in git.
Relocate IMPLEMENTATION_AUDIT.md and IMPLEMENTATION_NOTES.md next to the existing ADRs, and add pitch-finding notes and a status doc.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
staff-finding/dox/PITCH_FINDING_NOTES.md-149-151 (1)
149-151: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTypo: "shunted over at they go" → "as they go".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/dox/PITCH_FINDING_NOTES.md` around lines 149 - 151, Correct the typo in the note by replacing “at they go” with “as they go,” while preserving the surrounding wording.Source: Linters/SAST tools
staff-finding/dox/STATUS.md-31-40 (1)
31-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInternal contradiction: "all five implemented" vs
heatmap_regressionbeing design-doc only.Line 31 claims all five runners emit JSOMR, and line 34 says all five are implemented, but line 40 says heatmap regression is not yet implemented. Suggest "four of five implemented".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/dox/STATUS.md` around lines 31 - 40, Update the experiment-runner status wording to say four of five are implemented, while preserving the heatmap_regression entry as design-doc only and leaving the four implemented runner descriptions unchanged..gitignore-13-14 (1)
13-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNew ignore rules cover paths this PR actively tracks.
.gitignorenow ignores both.claude/andstaff-finding/e2e_tests, yet files under both are added or modified in this same PR. Git keeps already-tracked files tracked, so the rules take no effect today but will silently hide future changes in those paths — pick untrack-or-drop per path.
.gitignore#L13-L14: decide whetherstaff-finding/e2e_testsfixtures are intentional committed test data (drop the rule, or narrow it to the generatedrun_page*/subtrees) and add a trailing/if a directory match is intended..claude/settings.local.json#L16-L28:git rm --cachedthis file so the.claude/rule applies, since its absolute/Users/...and/opt/anaconda3/envs/kraken/...paths are machine-specific and unusable by other contributors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 13 - 14, Update .gitignore so intentional staff-finding/e2e_tests fixtures remain tracked, either remove that rule or narrow it to generated run_page*/ subtrees and use a trailing slash for directory matching. For .claude/settings.local.json, remove the file from Git’s index with git rm --cached while retaining the .claude/ ignore rule; no direct content change is needed there.staff-finding/dox/STATUS.md-22-24 (1)
22-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale:
interpolate_staves.pyis no longer just a placeholder.This PR ships
interpolate_missing_lines()with Trigger A (in-stave gap fill), Trigger B (territory-bounded edge extrapolation), and a rhythm gate, andgroup_staves.pycalls it wheninterpolate_missing=True. The accurate statement is that interpolation is implemented but off by default. Line 116 ("Interpolation stub is a no-op") and §1 at lines 49-54 carry the same stale claim. Since this doc is explicitly a handoff note, the mismatch is likely to send the next maintainer down the wrong path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/dox/STATUS.md` around lines 22 - 24, Update the STATUS.md handoff note to state that interpolation is implemented, including its existing triggers and rhythm gate, but disabled by default unless group_staves.py receives interpolate_missing=True. Replace the stale “placeholder,” “stub is a no-op,” and corresponding §1 claims throughout the document while preserving the accurate implementation details..gitignore-19-22 (1)
19-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
*.pton line 22 overrides the!models/**/*.ptexception.Git applies the last matching pattern, so adding
*.ptafter the negation re-ignores everything undermodels/, defeating line 21. Put the broad pattern before the negation.🔧 Proposed fix
# Downloaded pretrained weights yolov8*.pt -!models/**/*.pt *.pt +!models/**/*.pt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 19 - 22, Reorder the .gitignore patterns so the broad *.pt ignore rule appears before the !models/**/*.pt exception, preserving the exception for pretrained weights under models/ while ignoring other .pt files.staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.json-2-27 (1)
2-27: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRegenerate this fixture to include
centerline_page
staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.jsonstill stops atcenterline, whilestaff-finding/scripts/run_page.pynow writes acenterline_pageblock. Regenerating this fixture would make it a better regression anchor for the page-absolute coordinate path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.json` around lines 2 - 27, Regenerate the fixture represented by the stafflines JSON so each detected staff line includes the `centerline_page` block produced by `run_page.py`. Preserve the existing `centerline` data and ensure the regenerated output reflects page-absolute coordinates for the regression case.staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000_model-predicted.txt-1-1 (1)
1-1: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse class
2for stafflines here — this TXT and the sibling JSON are already aligned (class 2/classId: 3); the problem isstaff-finding/scripts/eval_page.pydefaultingstaffline_classto0, which drops all GT boxes and leaves only theno GT stafflineswarning unless--staffline_class 2is passed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000_model-predicted.txt` at line 1, Update the default staffline class in eval_page.py to 2 so evaluation uses the class represented by the TXT and sibling JSON; preserve the --staffline_class override for callers needing a different class and ensure GT staff boxes are no longer dropped by default.staff-finding/experiments/README.md-89-113 (1)
89-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate documentation to reflect implemented pipeline stages.
The experiment index labels implicit-neural and periodicity as planned, while the notes describe interpolation as stubbed and deferred. The supplied PR context includes implemented runners and interpolation support, so these instructions misrepresent available functionality.
staff-finding/experiments/README.md#L89-L113: mark implemented implicit-neural and periodicity methods as implemented and document their runners.staff-finding/experiments/implicit_neural/NOTES.md#L129-L142: replace the “currently stubbed” interpolation statement with the current implementation and usage details.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/README.md` around lines 89 - 113, Update staff-finding/experiments/README.md lines 89-113 to mark implicit_neural and periodicity as implemented, and document their available runners. Update staff-finding/experiments/implicit_neural/NOTES.md lines 129-142 to replace the stubbed/deferred interpolation description with the current implementation and usage details.staff-finding/experiments/dp_tracing/dp_tracer.py-75-89 (1)
75-89: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate fully out-of-bounds trace ranges.
x_startis not upper-clamped andx_endis not lower-clamped. An out-of-range detection can therefore return empty arrays here, then crash the runner when it accessesxs[0]. Clamp both endpoints to the image bounds and explicitly rejectx_end < x_start.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/dp_tracing/dp_tracer.py` around lines 75 - 89, The trace range validation must clamp both horizontal endpoints to image bounds and reject inverted ranges before constructing results. Update the range handling around x_start and x_end to lower-clamp x_start, upper-clamp x_end, and explicitly handle x_end < x_start by returning empty arrays, while preserving the existing band/column validation behavior.staff-finding/dox/IMPLEMENTATION_NOTES.md-73-73 (1)
73-73: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTypo: "Downstrem" → "Downstream".
✏️ Proposed fix
-- **Downstrem robustness**: Robust fitting handles local asymmetries; grouping doesn't need to +- **Downstream robustness**: Robust fitting handles local asymmetries; grouping doesn't need to🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/dox/IMPLEMENTATION_NOTES.md` at line 73, Correct the misspelled “Downstrem” heading in the implementation notes to “Downstream,” leaving the rest of the note unchanged.staff-finding/experiments/periodicity/periodicity_detector.py-224-264 (1)
224-264: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOff-by-one in upper-tooth row bound; unused clamp variables.
r_up_hisubtracts an extra-1compared to the mirrored lower-tooth formula (page_h - 1 - y_lo - offset_px), excluding the last valid band row from the upper-tooth comb contribution near the page boundary. Also,y_up_lo_c/y_up_hi_care computed but never used.🐛 Proposed fix
# Upper tooth: y_lo+r - offset_px - y_up_lo = y_lo - offset_px - y_up_hi = y_hi - offset_px - y_up_lo_c = max(0, y_up_lo) - y_up_hi_c = min(page_h - 1, y_up_hi) - # Which band rows have a valid upper tooth? # Band row r maps to page row y_lo + r. # Upper tooth page row: y_lo + r - offset_px. # Valid when 0 <= y_lo + r - offset_px <= page_h - 1 # i.e. offset_px <= r + y_lo and r + y_lo - offset_px <= page_h - 1 r_up_lo = max(0, offset_px - y_lo) # first band row with valid upper tooth - r_up_hi = min(n_band - 1, page_h - 1 - y_lo + offset_px - 1) + r_up_hi = min(n_band - 1, page_h - 1 - y_lo + offset_px)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/periodicity/periodicity_detector.py` around lines 224 - 264, Update the upper-tooth bounds in the periodicity comb loop by removing the extra subtraction from r_up_hi so the final valid band row is included, matching the lower-tooth boundary logic. Remove the unused y_up_lo_c and y_up_hi_c clamp variables, while preserving the existing slice and validity checks.staff-finding/experiments/shared_utils.py-105-113 (1)
105-113: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
write_jsomrnever appliesx_page_offset/y_page_offset.The
ExperimentFitResultdocstring promises page-absolute coordinates are obtained by adding the offsets tox_start/x_end/y_values, butwrite_jsomrwrites these fields verbatim. Currently harmless because every caller pre-computes page-absolute values and zeroes the offsets, but any future runner that populates crop-local coordinates with non-zero offsets (as the dataclass explicitly supports) will silently produce wrong page-absolute coordinates in the JSOMR output — the same class of bug this PR is fixing elsewhere in the pipeline.🐛 Proposed fix
"centerline": { - "x_start": fit.x_start, - "x_end": fit.x_end, - "y_values": [round(float(y), 1) for y in fit.y_values], + "x_start": fit.x_start + fit.x_page_offset, + "x_end": fit.x_end + fit.x_page_offset, + "y_values": [round(float(y) + fit.y_page_offset, 1) for y in fit.y_values], },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/shared_utils.py` around lines 105 - 113, Update write_jsomr when constructing the centerline record to add fit.x_page_offset to x_start and x_end, and fit.y_page_offset to every value in fit.y_values before writing JSOMR output. Preserve the existing rounding behavior for the resulting page-absolute y coordinates and leave unrelated bounding-box fields unchanged.staff-finding/scripts/fit_centerline.py-238-249 (1)
238-249: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefinement gate compares residuals over two different populations.
refined_residualsis measured over ~N window medians whileabs_residualsis measured over every kept pixel; medians are structurally much closer to any curve, so the guard passes almost unconditionally andline_following_no_improvementis close to unreachable. It also meansresidual_mean/residual_maxon the emittedFitResult(and the summary CSV / JSOMR) silently switch population when refinement fires, so QA can no longer compare them across boxes.Consider scoring the refined coefficients on the in-band pixels (same set the trace was drawn from) and reporting residuals over that set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/fit_centerline.py` around lines 238 - 249, The refinement gate in the fit-centerline flow must compare like-for-like residual populations. In the refinement block, evaluate refined_coeffs against the same kept in-band pixel coordinates used to compute abs_residuals, use that result for the improvement check, and retain/report residuals from that pixel set when refinement is accepted; keep trace_xs_out and trace_ys_out updates unchanged.staff-finding/scripts/eval_batch.py-146-148 (1)
146-148: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
--staffline-classnever overrides the manifest, contrary to its help text.argparse always supplies a value, so a present manifest column always wins. Either reword the help or default the flag to
Noneand treat a supplied flag as the override.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/eval_batch.py` around lines 146 - 148, Update the argument handling for staffline_class so --staffline-class defaults to None, allowing an explicitly supplied flag to override the manifest value while preserving the manifest value when the flag is omitted. Adjust the conversion logic around staffline_class and its argparse definition without changing unrelated behavior.staff-finding/scripts/group_staves.py-677-682 (1)
677-682: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStroke colours are BGR triples drawn into an RGB canvas.
Line 677 now converts the page to RGB, but the polyline colours are still built with
reversed(color_rgb).imshow(canvas_uint8)(and theRGB2BGRhq write) therefore render every stave line with R and B swapped, so the drawn lines no longer match thestave_colorslegend patches. Drop thereversed(...)for both the detected and interpolated strokes.🎨 Proposed fix (interpolated branch)
- color_bgr = tuple(int(c * 255) for c in reversed(light_rgb)) + color_rgb_255 = tuple(int(c * 255) for c in light_rgb)Also applies to: 748-752
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/group_staves.py` around lines 677 - 682, Update the detected and interpolated stroke-color construction in the relevant drawing logic to pass each `color_rgb` directly, removing `reversed(...)`. Keep the RGB canvas and existing legend colors unchanged so polylines match the `stave_colors` patches in both branches.
🧹 Nitpick comments (12)
staff-finding/dox/PITCH_FINDING_NOTES.md (1)
103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced block (markdownlint MD040).
textis fine for the ASCII pipeline diagram.📝 Proposed fix
-``` +```text Staff-finding output (JSOMR)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/dox/PITCH_FINDING_NOTES.md` at line 103, Add the text language identifier to the fenced code block containing the ASCII pipeline diagram in PITCH_FINDING_NOTES.md, changing the opening fence to use text while preserving the diagram content.Source: Linters/SAST tools
staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txt (1)
247-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value19 of 111 fits (~17%) dropped with
no_y_position_available.Worth capturing this rate as an explicit metric in the report header rather than leaving it implicit in the list length — it's the single most useful number for tracking component-filter regressions across runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txt` around lines 247 - 266, Update the stave grouping report header to explicitly include the count and percentage of fits dropped with no_y_position_available, using the existing unassigned-fit totals so it remains accurate across runs..gitignore (1)
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepo-wide
*.png/*.jpgignore will also swallow documentation/diagnostic images.If any README or
dox/assets are images, they'll need explicit!exceptions. Worth confirming before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 24 - 28, Review the repository for README or dox/ image assets that should remain tracked, then add explicit negation entries in .gitignore for those paths after the repo-wide *.png, *.jpg, and *.jpeg rules. Keep unrelated image files ignored..claude/settings.local.json (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMachine-specific local settings are being committed while
.claude/is simultaneously added to.gitignore.These entries hard-code one developer's home directory and conda env paths, so they carry no value for other contributors and will drift. Since
.gitignoreline 15 now ignores.claude/, this file is tracked-but-ignored — future edits will silently stop being staged. Considergit rm --cachedfor this file so the ignore rule actually takes effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/settings.local.json around lines 16 - 28, Remove the machine-specific .claude/settings.local.json from version control using git rm --cached, while retaining it locally so the existing .claude/ ignore rule applies. Do not add these user-specific paths or command permissions back to tracked configuration.staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.json (1)
14555-14560: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCenterlines extrapolate to negative y outside the crop.
line0050reaches-0.3andline0090starts at-3.2, i.e. the quadratic fit is evaluated well past the pixel support it was fitted on (residual_mean5.3 / 2.2,residual_max~15). Not a defect in this artifact, but it suggests centerline sampling isn't clamped to the box's valid y-range, which will feed bad rows into the pitch grid. Worth clamping or flagging at fit time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.json` around lines 14555 - 14560, Clamp centerline sampling to the fitted box’s valid y-range before generating pitch-grid rows, or flag fits that extrapolate beyond that range. Update the centerline-fit/sampling logic associated with line0050 and line0090, preserving valid in-range samples while preventing negative-y extrapolated rows from entering the pitch grid.README.md (1)
337-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language identifier to the manifest CSV fenced code block.
Static analysis flags this fence as missing a language hint.
📝 Proposed fix
-``` +```csv page_name,image,gt_txt,gt_source,pred_json,variant GentAnt1475_0017_AC_rightcrop,staff-finding/image-sets/gent/right/GentAnt1475_0017_AC_rightcrop.jpg,path/to/gt.txt,corrected_kyrie,path/to/stafflines.json,sauvola_no_bgr🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 337 - 340, Add the csv language identifier to the fenced manifest example in the README while preserving its contents and formatting.Source: Linters/SAST tools
staff-finding/experiments/shared_utils.py (1)
197-254: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider
zip(..., strict=True)for defensive length checking.
fit_resultsandboxesare always built together today, butstrict=Truewould make a future length mismatch fail loudly instead of silently truncating.♻️ Proposed fix
- for idx, (fit, box) in enumerate(zip(fit_results, boxes)): + for idx, (fit, box) in enumerate(zip(fit_results, boxes, strict=True)):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/shared_utils.py` around lines 197 - 254, Update the `write_jsomr` call in `run_grouping_and_save` to pair `fit_results` and `boxes` with `zip(..., strict=True)` before passing them onward, so mismatched lengths fail loudly instead of being silently truncated. Preserve the existing grouping and output behavior.Source: Linters/SAST tools
staff-finding/scripts/train_staffline_detector.py (1)
180-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the import failure.
raise SystemExit(...) from None(orfrom exc) keeps the traceback honest and clears Ruff B904.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/train_staffline_detector.py` around lines 180 - 185, Update the YOLO import error handling in the try/except block to explicitly chain the SystemExit exception using an appropriate from clause, such as from None or the caught ImportError, while preserving the existing installation guidance.Source: Linters/SAST tools
staff-finding/scripts/component_filter.py (2)
139-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
companion_labelsfield not documented in the class docstring.The
ComponentFilterResultdocstring (Attributes section above) enumeratesmerged_cluster_labelsbut doesn't mention the newcompanion_labelsfield.📝 Suggested docstring addition
merged_cluster_labels: List of connected-component labels that were merged into the active cluster (when merge_components=True); empty list otherwise. + companion_labels: List of connected-component labels retained as + companions of the active winner/cluster (score above + COMPANION_SCORE_FLOOR, non-overlapping x-range). """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/component_filter.py` around lines 139 - 151, Update the ComponentFilterResult class docstring’s Attributes section to document the companion_labels field alongside merged_cluster_labels, describing its purpose consistently with the field definition.
330-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo dedicated test exercises the new companion-retention path.
None of the reviewed test files (
c-filter_test.py,test_merge.py,test_merge_step.py) construct a scenario with a discarded "not_top_scoring" candidate scoring aboveCOMPANION_SCORE_FLOORwith a non-overlapping x-range, so the companion feature added here (Lines 334-403) has no direct regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/component_filter.py` around lines 330 - 403, Add focused regression coverage for the companion-retention logic in the code handling `survivors[1:]`: construct a discarded candidate whose score meets `COMPANION_SCORE_FLOOR` and whose x-range does not overlap the winner, then verify it is included in the active coordinates, mask, and companion labels for both merge and no-merge modes. Also cover the exclusion case for overlapping or below-floor candidates.staff-finding/scripts/script_tests/test_merge.py (1)
51-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_merge.pylooks superseded bytest_merge_step.py.Both files define
test_fragmented_line_merges/test_two_lines_do_not_mergeagainst the same helper crops, buttest_merge_step.py's versions additionally exercise themerge_componentsparameter andmerged_cluster_labelsadded by this PR, while this file's versions don't. Keeping both risks drift (only one gets updated as the API evolves). Consider retiring this file in favour oftest_merge_step.py, or clarifying why both are intentionally kept.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/script_tests/test_merge.py` around lines 51 - 91, Retire the duplicate tests in test_merge.py and rely on test_merge_step.py, whose test_fragmented_line_merges and test_two_lines_do_not_merge cases cover the merge_components and merged_cluster_labels behavior. Remove the superseded test definitions and any associated standalone execution path, unless there is a documented distinct purpose that requires preserving them.staff-finding/scripts/interpolate_staves.py (1)
252-266: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLatent index-misalignment risk between
centersandfit_pairs_sortedin Trigger B.
centersis derived by mapping_y_at_centeroverfit_pairs_sortedand then filtering outNone(Lines 253-254), butfit_pairs_sorteditself is never filtered.zip(centers, fit_pairs_sorted)at Line 308 then assumes positional correspondence between the two lists. If any entry infit_pairs_sortedever producedNonefrom_y_at_center, the filtering would silently shift indices and pair the wrong center with the wrong fit — a hard-to-detect corruption of the slot-matching logic. Today this appears benign only becausegroup_staves.pyseems to gate stave assignment on the same "has y_values" condition, so noNoneshould reach this point in practice — but that's an implicit cross-module invariant. Building(center, pair)tuples together and filtering jointly would remove the fragility.🛡️ Suggested defensive fix
- fit_pairs_sorted = sorted(fit_pairs, key=lambda p: _y_at_center(p[1]) or 0.0) - centers = [_y_at_center(f) for _, f in fit_pairs_sorted] - centers = [c for c in centers if c is not None] + fit_pairs_sorted = sorted(fit_pairs, key=lambda p: _y_at_center(p[1]) or 0.0) + paired = [(c, p) for p in fit_pairs_sorted if (c := _y_at_center(p[1])) is not None] + centers = [c for c, _ in paired] + fit_pairs_sorted = [p for _, p in paired]Also applies to: 299-311
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/interpolate_staves.py` around lines 252 - 266, In the Trigger B processing around fit_pairs_sorted and the later zip, preserve center-to-fit alignment by computing each _y_at_center result together with its corresponding fit pair and filtering out entries whose center is None from both collections. Use the jointly filtered pairs to derive centers and for subsequent slot matching, while preserving the existing empty-center handling and gap estimation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txt`:
- Around line 5-8: The Stage 6 line-count tally in group_staves.py must collapse
or discard adjacent staffline assignments whose gaps are below min_threshold
before computing the mode and staves_with_unexpected_count flags. Update the
staff-grouping flow so near-duplicate detections, including clustered six-line
assignments, count as one actual staffline while preserving distinct lines
separated by at least min_threshold.
In
`@staff-finding/e2e_tests/pitch_finding_sample/GentAnt1475_0017_AC_rightcrop_no_bgr/stave_grouping_report.txt`:
- Around line 5-8: Regenerate the stave_grouping_report fixture after fixing the
split logic so it represents a successful grouping result rather than fragmented
staves. Update the e2e validation to assert acceptable stave line counts,
ensuring the regenerated baseline no longer contains the current
unexpected-count flags or 1–2-line fragments.
In `@staff-finding/experiments/gp_centerlines/gp_fitter.py`:
- Around line 128-136: Replace the fabricated mean-y fallback in the GP fitting
function around gpr.fit with an explicit failed-fit result that preserves the
exception details and cannot be interpreted as a normal prediction. Update the
runner’s handling of this result to flag the fit as failed and exclude it from
stave grouping and exported predictions, rather than consuming its centreline
and uncertainty arrays.
In `@staff-finding/scripts/component_filter.py`:
- Around line 334-403: Add a vertical center-distance check to companion
retention in the survivor loop, using the existing merge y-threshold and the
winner/candidate y-center values, and require it alongside the current
non-overlapping x-range checks for both no-merge and merge companions. Preserve
the existing score and merged-winner exclusions.
In `@staff-finding/scripts/eval_batch.py`:
- Around line 85-93: Update the metric filtering in the aggregation loop around
metrics_to_agg to exclude parsed non-finite float values, including
float("nan"), before constructing the NumPy array. Preserve valid numeric values
and the existing skip behavior when no values remain, so mean, standard
deviation, minimum, and maximum are computed only from finite metrics.
In `@staff-finding/scripts/eval_page.py`:
- Around line 59-71: Update _pred_page_y and _pred_page_x to prefer the
serialized centerline_page coordinates when present, without applying
bounding_box offsets to those page-absolute values. Retain the existing
centerline plus bounding_box conversion only for legacy crop-local data when
that contract is guaranteed, so page coordinates are not offset twice.
In `@staff-finding/scripts/group_staves.py`:
- Around line 733-746: Update the interpolated-line construction in
interpolate_staves.py to store page-absolute x coordinates by adding each fit’s
x_page_offset to f.x_start and f.x_end, matching the existing page-absolute
y_values behavior. Preserve integer conversion and ensure the resulting
InterpolatedLine fields, _reindex_stave_lines comparisons, JSON export, and
overlay logic use the corrected coordinates.
In `@staff-finding/scripts/interpolate_staves.py`:
- Around line 106-136: The interpolated-line construction paths must convert
crop-local x coordinates to page-absolute coordinates. Locate both branches that
create InterpolatedLine, including the range using
above_f.x_start/below_f.x_start and the branch using ref_f.x_start, and add the
corresponding x_page_offset before passing x-range values; preserve the existing
y interpolation behavior.
In `@staff-finding/scripts/script_tests/test_run_pageOG.py`:
- Around line 15-17: Update the import setup in test_run_pageOG.py to resolve
staff-finding/scripts/run_pageOG.py from the test file’s __file__ location,
replacing the hard-coded /home/claude/run_page.py path. Ensure the loaded module
targets the OG driver while remaining valid across repository checkouts.
In `@staff-finding/scripts/train_staffline_detector.py`:
- Around line 97-110: Update remap_label_file to discard every label whose class
is not SOURCE_CLASS, and only write retained SOURCE_CLASS entries using
TARGET_CLASS. Preserve the existing handling of blank lines and label
coordinates so the generated single-class dataset contains no class-0 or class-1
boxes.
---
Minor comments:
In @.gitignore:
- Around line 13-14: Update .gitignore so intentional staff-finding/e2e_tests
fixtures remain tracked, either remove that rule or narrow it to generated
run_page*/ subtrees and use a trailing slash for directory matching. For
.claude/settings.local.json, remove the file from Git’s index with git rm
--cached while retaining the .claude/ ignore rule; no direct content change is
needed there.
- Around line 19-22: Reorder the .gitignore patterns so the broad *.pt ignore
rule appears before the !models/**/*.pt exception, preserving the exception for
pretrained weights under models/ while ignoring other .pt files.
In `@staff-finding/dox/IMPLEMENTATION_NOTES.md`:
- Line 73: Correct the misspelled “Downstrem” heading in the implementation
notes to “Downstream,” leaving the rest of the note unchanged.
In `@staff-finding/dox/PITCH_FINDING_NOTES.md`:
- Around line 149-151: Correct the typo in the note by replacing “at they go”
with “as they go,” while preserving the surrounding wording.
In `@staff-finding/dox/STATUS.md`:
- Around line 31-40: Update the experiment-runner status wording to say four of
five are implemented, while preserving the heatmap_regression entry as
design-doc only and leaving the four implemented runner descriptions unchanged.
- Around line 22-24: Update the STATUS.md handoff note to state that
interpolation is implemented, including its existing triggers and rhythm gate,
but disabled by default unless group_staves.py receives
interpolate_missing=True. Replace the stale “placeholder,” “stub is a no-op,”
and corresponding §1 claims throughout the document while preserving the
accurate implementation details.
In
`@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000_model-predicted.txt`:
- Line 1: Update the default staffline class in eval_page.py to 2 so evaluation
uses the class represented by the TXT and sibling JSON; preserve the
--staffline_class override for callers needing a different class and ensure GT
staff boxes are no longer dropped by default.
In
`@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.json`:
- Around line 2-27: Regenerate the fixture represented by the stafflines JSON so
each detected staff line includes the `centerline_page` block produced by
`run_page.py`. Preserve the existing `centerline` data and ensure the
regenerated output reflects page-absolute coordinates for the regression case.
In `@staff-finding/experiments/dp_tracing/dp_tracer.py`:
- Around line 75-89: The trace range validation must clamp both horizontal
endpoints to image bounds and reject inverted ranges before constructing
results. Update the range handling around x_start and x_end to lower-clamp
x_start, upper-clamp x_end, and explicitly handle x_end < x_start by returning
empty arrays, while preserving the existing band/column validation behavior.
In `@staff-finding/experiments/periodicity/periodicity_detector.py`:
- Around line 224-264: Update the upper-tooth bounds in the periodicity comb
loop by removing the extra subtraction from r_up_hi so the final valid band row
is included, matching the lower-tooth boundary logic. Remove the unused
y_up_lo_c and y_up_hi_c clamp variables, while preserving the existing slice and
validity checks.
In `@staff-finding/experiments/README.md`:
- Around line 89-113: Update staff-finding/experiments/README.md lines 89-113 to
mark implicit_neural and periodicity as implemented, and document their
available runners. Update staff-finding/experiments/implicit_neural/NOTES.md
lines 129-142 to replace the stubbed/deferred interpolation description with the
current implementation and usage details.
In `@staff-finding/experiments/shared_utils.py`:
- Around line 105-113: Update write_jsomr when constructing the centerline
record to add fit.x_page_offset to x_start and x_end, and fit.y_page_offset to
every value in fit.y_values before writing JSOMR output. Preserve the existing
rounding behavior for the resulting page-absolute y coordinates and leave
unrelated bounding-box fields unchanged.
In `@staff-finding/scripts/eval_batch.py`:
- Around line 146-148: Update the argument handling for staffline_class so
--staffline-class defaults to None, allowing an explicitly supplied flag to
override the manifest value while preserving the manifest value when the flag is
omitted. Adjust the conversion logic around staffline_class and its argparse
definition without changing unrelated behavior.
In `@staff-finding/scripts/fit_centerline.py`:
- Around line 238-249: The refinement gate in the fit-centerline flow must
compare like-for-like residual populations. In the refinement block, evaluate
refined_coeffs against the same kept in-band pixel coordinates used to compute
abs_residuals, use that result for the improvement check, and retain/report
residuals from that pixel set when refinement is accepted; keep trace_xs_out and
trace_ys_out updates unchanged.
In `@staff-finding/scripts/group_staves.py`:
- Around line 677-682: Update the detected and interpolated stroke-color
construction in the relevant drawing logic to pass each `color_rgb` directly,
removing `reversed(...)`. Keep the RGB canvas and existing legend colors
unchanged so polylines match the `stave_colors` patches in both branches.
---
Nitpick comments:
In @.claude/settings.local.json:
- Around line 16-28: Remove the machine-specific .claude/settings.local.json
from version control using git rm --cached, while retaining it locally so the
existing .claude/ ignore rule applies. Do not add these user-specific paths or
command permissions back to tracked configuration.
In @.gitignore:
- Around line 24-28: Review the repository for README or dox/ image assets that
should remain tracked, then add explicit negation entries in .gitignore for
those paths after the repo-wide *.png, *.jpg, and *.jpeg rules. Keep unrelated
image files ignored.
In `@README.md`:
- Around line 337-340: Add the csv language identifier to the fenced manifest
example in the README while preserving its contents and formatting.
In `@staff-finding/dox/PITCH_FINDING_NOTES.md`:
- Line 103: Add the text language identifier to the fenced code block containing
the ASCII pipeline diagram in PITCH_FINDING_NOTES.md, changing the opening fence
to use text while preserving the diagram content.
In
`@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.json`:
- Around line 14555-14560: Clamp centerline sampling to the fitted box’s valid
y-range before generating pitch-grid rows, or flag fits that extrapolate beyond
that range. Update the centerline-fit/sampling logic associated with line0050
and line0090, preserving valid in-range samples while preventing negative-y
extrapolated rows from entering the pitch grid.
In
`@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txt`:
- Around line 247-266: Update the stave grouping report header to explicitly
include the count and percentage of fits dropped with no_y_position_available,
using the existing unassigned-fit totals so it remains accurate across runs.
In `@staff-finding/experiments/shared_utils.py`:
- Around line 197-254: Update the `write_jsomr` call in `run_grouping_and_save`
to pair `fit_results` and `boxes` with `zip(..., strict=True)` before passing
them onward, so mismatched lengths fail loudly instead of being silently
truncated. Preserve the existing grouping and output behavior.
In `@staff-finding/scripts/component_filter.py`:
- Around line 139-151: Update the ComponentFilterResult class docstring’s
Attributes section to document the companion_labels field alongside
merged_cluster_labels, describing its purpose consistently with the field
definition.
- Around line 330-403: Add focused regression coverage for the
companion-retention logic in the code handling `survivors[1:]`: construct a
discarded candidate whose score meets `COMPANION_SCORE_FLOOR` and whose x-range
does not overlap the winner, then verify it is included in the active
coordinates, mask, and companion labels for both merge and no-merge modes. Also
cover the exclusion case for overlapping or below-floor candidates.
In `@staff-finding/scripts/interpolate_staves.py`:
- Around line 252-266: In the Trigger B processing around fit_pairs_sorted and
the later zip, preserve center-to-fit alignment by computing each _y_at_center
result together with its corresponding fit pair and filtering out entries whose
center is None from both collections. Use the jointly filtered pairs to derive
centers and for subsequent slot matching, while preserving the existing
empty-center handling and gap estimation behavior.
In `@staff-finding/scripts/script_tests/test_merge.py`:
- Around line 51-91: Retire the duplicate tests in test_merge.py and rely on
test_merge_step.py, whose test_fragmented_line_merges and
test_two_lines_do_not_merge cases cover the merge_components and
merged_cluster_labels behavior. Remove the superseded test definitions and any
associated standalone execution path, unless there is a documented distinct
purpose that requires preserving them.
In `@staff-finding/scripts/train_staffline_detector.py`:
- Around line 180-185: Update the YOLO import error handling in the try/except
block to explicitly chain the SystemExit exception using an appropriate from
clause, such as from None or the caught ImportError, while preserving the
existing installation guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+5
to
+8
| Mode lines per stave: 2 | ||
| Line count distribution: {2: 9, 3: 3, 4: 2, 1: 1, 6: 4, 8: 1, 7: 2, 5: 2} | ||
| Cut threshold (px): 17.0 | ||
| Flags: mode_count_below_typical, staves_with_unexpected_count:2,5,7,8,10,11,12,15,16,22,23 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C5 'MIN_GAP_MULTIPLIER|def _determine_cut_threshold|def _find_valley_threshold' staff-finding/scripts/group_staves.pyRepository: DDMAL/mothra
Length of output: 2661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant thresholding and grouping logic.
sed -n '220,320p' staff-finding/scripts/group_staves.py
printf '\n---\n'
sed -n '430,520p' staff-finding/scripts/group_staves.py
printf '\n---\n'
sed -n '520,700p' staff-finding/scripts/group_staves.pyRepository: DDMAL/mothra
Length of output: 14866
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the fixture report around the cited lines and surrounding diagnostics.
sed -n '1,120p' staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txtRepository: DDMAL/mothra
Length of output: 3126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'duplicate|dedup|count.*stave|line count|unexpected_count|interpolate|normal|valley_threshold|MIN_GAP_MULTIPLIER|cut_threshold' staff-finding/scripts/group_staves.pyRepository: DDMAL/mothra
Length of output: 20012
Collapse near-duplicate stafflines before Stage 6 in staff-finding/scripts/group_staves.py
The 0.0–0.2 px gaps here and the 6-line cluster in Stave 7 are duplicate detections, not distinct stafflines. Stage 6 still counts every raw assignment, so those duplicates pull the page mode down to 2 and trigger staves_with_unexpected_count. Merge or drop gaps below min_threshold before the line-count tally so the mode and rhythm flags reflect actual stafflines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txt`
around lines 5 - 8, The Stage 6 line-count tally in group_staves.py must
collapse or discard adjacent staffline assignments whose gaps are below
min_threshold before computing the mode and staves_with_unexpected_count flags.
Update the staff-grouping flow so near-duplicate detections, including clustered
six-line assignments, count as one actual staffline while preserving distinct
lines separated by at least min_threshold.
Comment on lines
+5
to
+8
| Mode lines per stave: 8 | ||
| Line count distribution: {6: 2, 8: 5, 9: 1, 1: 3, 2: 1, 4: 3, 5: 1, 3: 1} | ||
| Cut threshold (px): 15.0 | ||
| Flags: staves_with_unexpected_count:0,5,6,8,9,10,11,13,14,15,16 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not retain fragmented grouping as the e2e baseline.
The reported mode is eight lines, but 11 of 17 staves are flagged as unexpected and several contain only 1–2 fits. This fixture records a failed grouping result rather than validating the grouping fix. Regenerate it after correcting the split logic and assert acceptable stave counts in the e2e check.
Also applies to: 150-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@staff-finding/e2e_tests/pitch_finding_sample/GentAnt1475_0017_AC_rightcrop_no_bgr/stave_grouping_report.txt`
around lines 5 - 8, Regenerate the stave_grouping_report fixture after fixing
the split logic so it represents a successful grouping result rather than
fragmented staves. Update the e2e validation to assert acceptable stave line
counts, ensuring the regenerated baseline no longer contains the current
unexpected-count flags or 1–2-line fragments.
Comment on lines
+128
to
+136
| try: | ||
| gpr.fit(obs_x, obs_y) | ||
| except Exception as exc: | ||
| n = x_query_end - x_query_start + 1 | ||
| return ( | ||
| list(np.full(n, float(np.mean(obs_y)))), | ||
| list(np.full(n, float("inf"))), | ||
| {"error": str(exc)}, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not substitute a fabricated centreline on GP failure.
This fallback returns a flat mean-y fit, and the runner treats it as a normal result. A numerical or configuration failure therefore silently pollutes stave grouping and exported predictions. Return an explicit failed-fit result and have the runner flag and exclude it from grouping.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 130-130: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/experiments/gp_centerlines/gp_fitter.py` around lines 128 -
136, Replace the fabricated mean-y fallback in the GP fitting function around
gpr.fit with an explicit failed-fit result that preserves the exception details
and cannot be interpreted as a normal prediction. Update the runner’s handling
of this result to flag the fit as failed and exclude it from stave grouping and
exported predictions, rather than consuming its centreline and uncertainty
arrays.
Comment on lines
+334
to
+403
| # --- Companion retention --- | ||
| # Scan "not_top_scoring" losers. Keep any that: | ||
| # (a) score >= COMPANION_SCORE_FLOOR, and | ||
| # (b) have non-overlapping x-range with the active winner | ||
| # (horizontal continuation, not a parallel duplicate / adjacent line). | ||
| # Companion pixels are added to both the no-merge and merge coord/mask sets | ||
| # so the downstream fit sees the full staffline extent. | ||
|
|
||
| no_merge_win_x_start = winner_stats["x"] | ||
| no_merge_win_x_end = winner_stats["x"] + winner_stats["w"] | ||
|
|
||
| if merged_scored: | ||
| m_stats_top = merged_scored[0][3] # (cluster, score, sub_scores, stats) | ||
| merge_win_x_start = m_stats_top["x"] | ||
| merge_win_x_end = m_stats_top["x"] + m_stats_top["w"] | ||
| else: | ||
| merge_win_x_start = no_merge_win_x_start | ||
| merge_win_x_end = no_merge_win_x_end | ||
|
|
||
| merged_winner_set = set(int(lbl) for lbl in merged_winner_labels) | ||
|
|
||
| no_merge_companion_labels: list[int] = [] | ||
| no_merge_companion_coords: list[tuple[int, int]] = [] | ||
| no_merge_companion_mask = np.zeros_like(labels, dtype=bool) | ||
|
|
||
| merge_companion_labels: list[int] = [] | ||
| merge_companion_coords: list[tuple[int, int]] = [] | ||
| merge_companion_mask = np.zeros_like(labels, dtype=bool) | ||
|
|
||
| for s in survivors[1:]: | ||
| s_label, s_score, _s_sub, s_stats = s | ||
| if s_score < COMPANION_SCORE_FLOOR: | ||
| continue | ||
| comp_x = s_stats["x"] | ||
| comp_w = s_stats["w"] | ||
| comp_mask = labels == s_label | ||
| c_ys, c_xs = np.where(comp_mask) | ||
| comp_pixels = list(zip(c_xs.tolist(), c_ys.tolist())) | ||
|
|
||
| # No-merge companion: non-overlapping with no-merge winner bbox. | ||
| no_overlap_no_merge = (comp_x + comp_w <= no_merge_win_x_start) or ( | ||
| comp_x >= no_merge_win_x_end | ||
| ) | ||
| if no_overlap_no_merge: | ||
| no_merge_companion_labels.append(int(s_label)) | ||
| no_merge_companion_mask |= comp_mask | ||
| no_merge_companion_coords.extend(comp_pixels) | ||
|
|
||
| # Merge companion: must not already be in merged winner; non-overlapping | ||
| # with merged winner's combined bbox. | ||
| if int(s_label) not in merged_winner_set: | ||
| no_overlap_merge = (comp_x + comp_w <= merge_win_x_start) or ( | ||
| comp_x >= merge_win_x_end | ||
| ) | ||
| if no_overlap_merge: | ||
| merge_companion_labels.append(int(s_label)) | ||
| merge_companion_mask |= comp_mask | ||
| merge_companion_coords.extend(comp_pixels) | ||
|
|
||
| # --- Choose active outputs based on the mode --- | ||
| if merge_components: | ||
| active_coords = merged_coords | ||
| active_mask = merged_mask | ||
| active_coords = merged_coords + merge_companion_coords | ||
| active_mask = merged_mask | merge_companion_mask | ||
| active_flags = merge_flags | ||
| active_companion_labels = merge_companion_labels | ||
| else: | ||
| active_coords = no_merge_coords | ||
| active_mask = no_merge_mask | ||
| active_coords = no_merge_coords + no_merge_companion_coords | ||
| active_mask = no_merge_mask | no_merge_companion_mask | ||
| active_flags = no_merge_flags | ||
| active_companion_labels = no_merge_companion_labels |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Companion retention has no vertical-proximity check, unlike merge clustering.
_compute_merge_groups() requires both y-center proximity and x-gap to merge two components (Lines 592-604). The new companion-retention logic (Lines 363-391) only checks that a candidate's x-range doesn't overlap the winner's — there's no check that the candidate is vertically close to the winner. The docstring/comment (Lines 335-341) claims this constraint rules out "a parallel duplicate or adjacent staffline fragment," but non-overlapping x-range alone doesn't guarantee that: a fragment of a genuinely different (adjacent) staffline that happens to sit outside the winner's x-range could score above COMPANION_SCORE_FLOOR (0.25) and get folded into coords/mask, contaminating the pixel set that fit_centerline consumes.
Given the same file already documents the page-arch/adjacent-staffline risk extensively for the merge thresholds, consider mirroring that same y-center distance guard here.
🛡️ Suggested guard using the existing y-threshold
+ y_threshold = MERGE_Y_CENTER_DISTANCE_MULTIPLIER * scale_unit
+ winner_y_center = winner_stats["y"] + winner_stats["h"] / 2.0
+
for s in survivors[1:]:
s_label, s_score, _s_sub, s_stats = s
if s_score < COMPANION_SCORE_FLOOR:
continue
+ comp_y_center = s_stats["y"] + s_stats["h"] / 2.0
+ if abs(comp_y_center - winner_y_center) > y_threshold:
+ continue
comp_x = s_stats["x"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # --- Companion retention --- | |
| # Scan "not_top_scoring" losers. Keep any that: | |
| # (a) score >= COMPANION_SCORE_FLOOR, and | |
| # (b) have non-overlapping x-range with the active winner | |
| # (horizontal continuation, not a parallel duplicate / adjacent line). | |
| # Companion pixels are added to both the no-merge and merge coord/mask sets | |
| # so the downstream fit sees the full staffline extent. | |
| no_merge_win_x_start = winner_stats["x"] | |
| no_merge_win_x_end = winner_stats["x"] + winner_stats["w"] | |
| if merged_scored: | |
| m_stats_top = merged_scored[0][3] # (cluster, score, sub_scores, stats) | |
| merge_win_x_start = m_stats_top["x"] | |
| merge_win_x_end = m_stats_top["x"] + m_stats_top["w"] | |
| else: | |
| merge_win_x_start = no_merge_win_x_start | |
| merge_win_x_end = no_merge_win_x_end | |
| merged_winner_set = set(int(lbl) for lbl in merged_winner_labels) | |
| no_merge_companion_labels: list[int] = [] | |
| no_merge_companion_coords: list[tuple[int, int]] = [] | |
| no_merge_companion_mask = np.zeros_like(labels, dtype=bool) | |
| merge_companion_labels: list[int] = [] | |
| merge_companion_coords: list[tuple[int, int]] = [] | |
| merge_companion_mask = np.zeros_like(labels, dtype=bool) | |
| for s in survivors[1:]: | |
| s_label, s_score, _s_sub, s_stats = s | |
| if s_score < COMPANION_SCORE_FLOOR: | |
| continue | |
| comp_x = s_stats["x"] | |
| comp_w = s_stats["w"] | |
| comp_mask = labels == s_label | |
| c_ys, c_xs = np.where(comp_mask) | |
| comp_pixels = list(zip(c_xs.tolist(), c_ys.tolist())) | |
| # No-merge companion: non-overlapping with no-merge winner bbox. | |
| no_overlap_no_merge = (comp_x + comp_w <= no_merge_win_x_start) or ( | |
| comp_x >= no_merge_win_x_end | |
| ) | |
| if no_overlap_no_merge: | |
| no_merge_companion_labels.append(int(s_label)) | |
| no_merge_companion_mask |= comp_mask | |
| no_merge_companion_coords.extend(comp_pixels) | |
| # Merge companion: must not already be in merged winner; non-overlapping | |
| # with merged winner's combined bbox. | |
| if int(s_label) not in merged_winner_set: | |
| no_overlap_merge = (comp_x + comp_w <= merge_win_x_start) or ( | |
| comp_x >= merge_win_x_end | |
| ) | |
| if no_overlap_merge: | |
| merge_companion_labels.append(int(s_label)) | |
| merge_companion_mask |= comp_mask | |
| merge_companion_coords.extend(comp_pixels) | |
| # --- Choose active outputs based on the mode --- | |
| if merge_components: | |
| active_coords = merged_coords | |
| active_mask = merged_mask | |
| active_coords = merged_coords + merge_companion_coords | |
| active_mask = merged_mask | merge_companion_mask | |
| active_flags = merge_flags | |
| active_companion_labels = merge_companion_labels | |
| else: | |
| active_coords = no_merge_coords | |
| active_mask = no_merge_mask | |
| active_coords = no_merge_coords + no_merge_companion_coords | |
| active_mask = no_merge_mask | no_merge_companion_mask | |
| active_flags = no_merge_flags | |
| active_companion_labels = no_merge_companion_labels | |
| # --- Companion retention --- | |
| # Scan "not_top_scoring" losers. Keep any that: | |
| # (a) score >= COMPANION_SCORE_FLOOR, and | |
| # (b) have non-overlapping x-range with the active winner | |
| # (horizontal continuation, not a parallel duplicate / adjacent line). | |
| # Companion pixels are added to both the no-merge and merge coord/mask sets | |
| # so the downstream fit sees the full staffline extent. | |
| no_merge_win_x_start = winner_stats["x"] | |
| no_merge_win_x_end = winner_stats["x"] + winner_stats["w"] | |
| if merged_scored: | |
| m_stats_top = merged_scored[0][3] # (cluster, score, sub_scores, stats) | |
| merge_win_x_start = m_stats_top["x"] | |
| merge_win_x_end = m_stats_top["x"] + m_stats_top["w"] | |
| else: | |
| merge_win_x_start = no_merge_win_x_start | |
| merge_win_x_end = no_merge_win_x_end | |
| merged_winner_set = set(int(lbl) for lbl in merged_winner_labels) | |
| no_merge_companion_labels: list[int] = [] | |
| no_merge_companion_coords: list[tuple[int, int]] = [] | |
| no_merge_companion_mask = np.zeros_like(labels, dtype=bool) | |
| merge_companion_labels: list[int] = [] | |
| merge_companion_coords: list[tuple[int, int]] = [] | |
| merge_companion_mask = np.zeros_like(labels, dtype=bool) | |
| y_threshold = MERGE_Y_CENTER_DISTANCE_MULTIPLIER * scale_unit | |
| winner_y_center = winner_stats["y"] + winner_stats["h"] / 2.0 | |
| for s in survivors[1:]: | |
| s_label, s_score, _s_sub, s_stats = s | |
| if s_score < COMPANION_SCORE_FLOOR: | |
| continue | |
| comp_y_center = s_stats["y"] + s_stats["h"] / 2.0 | |
| if abs(comp_y_center - winner_y_center) > y_threshold: | |
| continue | |
| comp_x = s_stats["x"] | |
| comp_w = s_stats["w"] | |
| comp_mask = labels == s_label | |
| c_ys, c_xs = np.where(comp_mask) | |
| comp_pixels = list(zip(c_xs.tolist(), c_ys.tolist())) | |
| # No-merge companion: non-overlapping with no-merge winner bbox. | |
| no_overlap_no_merge = (comp_x + comp_w <= no_merge_win_x_start) or ( | |
| comp_x >= no_merge_win_x_end | |
| ) | |
| if no_overlap_no_merge: | |
| no_merge_companion_labels.append(int(s_label)) | |
| no_merge_companion_mask |= comp_mask | |
| no_merge_companion_coords.extend(comp_pixels) | |
| # Merge companion: must not already be in merged winner; non-overlapping | |
| # with merged winner's combined bbox. | |
| if int(s_label) not in merged_winner_set: | |
| no_overlap_merge = (comp_x + comp_w <= merge_win_x_start) or ( | |
| comp_x >= merge_win_x_end | |
| ) | |
| if no_overlap_merge: | |
| merge_companion_labels.append(int(s_label)) | |
| merge_companion_mask |= comp_mask | |
| merge_companion_coords.extend(comp_pixels) | |
| # --- Choose active outputs based on the mode --- | |
| if merge_components: | |
| active_coords = merged_coords + merge_companion_coords | |
| active_mask = merged_mask | merge_companion_mask | |
| active_flags = merge_flags | |
| active_companion_labels = merge_companion_labels | |
| else: | |
| active_coords = no_merge_coords + no_merge_companion_coords | |
| active_mask = no_merge_mask | no_merge_companion_mask | |
| active_flags = no_merge_flags | |
| active_companion_labels = no_merge_companion_labels |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 371-371: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/component_filter.py` around lines 334 - 403, Add a
vertical center-distance check to companion retention in the survivor loop,
using the existing merge y-threshold and the winner/candidate y-center values,
and require it alongside the current non-overlapping x-range checks for both
no-merge and merge companions. Preserve the existing score and merged-winner
exclusions.
Comment on lines
+85
to
+93
| for m in metrics_to_agg: | ||
| vals = [float(r[m]) for r in rows if r[m] not in ("", "nan")] | ||
| if not vals: | ||
| continue | ||
| a = np.array(vals) | ||
| print( | ||
| f" {m:<20s} {a.mean():.4f} ± {a.std():.4f}" | ||
| f" [min {a.min():.4f} max {a.max():.4f}]" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
NaN metrics are not filtered out, so aggregates collapse to NaN.
evaluate() returns real floats, and split_ratio / mean_y_mae_px can be float("nan"). float("nan") not in ("", "nan") is True, so a single page without matches poisons the mean/std/min/max for that whole variant.
🐛 Proposed fix
- vals = [float(r[m]) for r in rows if r[m] not in ("", "nan")]
+ vals = [
+ v
+ for v in (
+ float(r[m]) for r in rows if r[m] not in ("", None, "nan")
+ )
+ if not np.isnan(v)
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for m in metrics_to_agg: | |
| vals = [float(r[m]) for r in rows if r[m] not in ("", "nan")] | |
| if not vals: | |
| continue | |
| a = np.array(vals) | |
| print( | |
| f" {m:<20s} {a.mean():.4f} ± {a.std():.4f}" | |
| f" [min {a.min():.4f} max {a.max():.4f}]" | |
| ) | |
| for m in metrics_to_agg: | |
| vals = [ | |
| v | |
| for v in ( | |
| float(r[m]) for r in rows if r[m] not in ("", None, "nan") | |
| ) | |
| if not np.isnan(v) | |
| ] | |
| if not vals: | |
| continue | |
| a = np.array(vals) | |
| print( | |
| f" {m:<20s} {a.mean():.4f} ± {a.std():.4f}" | |
| f" [min {a.min():.4f} max {a.max():.4f}]" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/eval_batch.py` around lines 85 - 93, Update the metric
filtering in the aggregation loop around metrics_to_agg to exclude parsed
non-finite float values, including float("nan"), before constructing the NumPy
array. Preserve valid numeric values and the existing skip behavior when no
values remain, so mean, standard deviation, minimum, and maximum are computed
only from finite metrics.
Comment on lines
+59
to
+71
| def _pred_page_y(item: dict) -> np.ndarray: | ||
| """Page-absolute y-values for a JSOMR staffline item.""" | ||
| return ( | ||
| np.array(item["centerline"]["y_values"], dtype=float) | ||
| + item["bounding_box"]["uly"] | ||
| ) | ||
|
|
||
|
|
||
| def _pred_page_x(item: dict) -> np.ndarray: | ||
| """Page-absolute x positions corresponding to each y_value.""" | ||
| x_start = item["centerline"]["x_start"] | ||
| n = len(item["centerline"]["y_values"]) | ||
| return np.arange(x_start, x_start + n) + item["bounding_box"]["ulx"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Which writers emit centerline / centerline_page, and what do they put in bounding_box?
rg -nP -C4 '"centerline(_page)?"\s*:' --type=py
rg -nP -C6 'ExperimentFitResult\(' staff-finding/experiments --type=py | head -80
fd -t f 'shared_utils.py' staff-finding --exec rg -nP -C5 '"bounding_box"|json\.dump' {}Repository: DDMAL/mothra
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files of interest =="
git ls-files 'staff-finding/**' | rg '(^staff-finding/scripts/eval_page.py$|^staff-finding/.*run_page.py$|^staff-finding/.*shared_utils.py$|^staff-finding/experiments/)'
echo
echo "== outline eval_page.py =="
ast-grep outline staff-finding/scripts/eval_page.py --view expanded || true
echo
echo "== relevant slices in eval_page.py =="
sed -n '1,220p' staff-finding/scripts/eval_page.py
echo
echo "== search for centerline_page and ExperimentFitResult =="
rg -n 'centerline_page|ExperimentFitResult|bounding_box|y_page_offset|x_page_offset' staff-finding -SRepository: DDMAL/mothra
Length of output: 16099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'staff-finding/**' | rg 'eval_page.py|run_page.py|shared_utils.py|experiments/.+\.py'
printf '\n== eval_page.py around lines 1-120 ==\n'
sed -n '1,120p' staff-finding/scripts/eval_page.py
printf '\n== run_page.py references ==\n'
rg -n 'centerline_page|bounding_box|y_page_offset|x_page_offset|page-absolute|x_start|y_values' staff-finding/scripts/run_page.py
printf '\n== experiment writer references ==\n'
rg -n 'ExperimentFitResult|bounding_box|centerline|y_page_offset|x_page_offset' staff-finding/experiments -SRepository: DDMAL/mothra
Length of output: 12397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared_utils.py relevant slices =="
sed -n '1,280p' staff-finding/experiments/shared_utils.py
echo
echo "== gp_centerlines runner slice =="
sed -n '130,210p' staff-finding/experiments/gp_centerlines/run_gp_page.py
echo
echo "== dp_tracing runner slice =="
sed -n '80,120p' staff-finding/experiments/dp_tracing/run_dp_page.py
echo
echo "== periodicity runner slice =="
sed -n '160,190p' staff-finding/experiments/periodicity/run_periodicity_page.py
echo
echo "== implicit_neural runner slice =="
sed -n '160,190p' staff-finding/experiments/implicit_neural/run_implicit_neural_page.pyRepository: DDMAL/mothra
Length of output: 15569
Teach the evaluator which coordinate space it’s reading
centerline + bounding_box only works for crop-local fits. The experiment writers serialize page-absolute y_values into the same shape, so bounding_box.uly gets added twice and the page metrics drift. Prefer centerline_page when it exists, and treat the legacy fields as crop-local only when the writer guarantees that contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/eval_page.py` around lines 59 - 71, Update _pred_page_y
and _pred_page_x to prefer the serialized centerline_page coordinates when
present, without applying bounding_box offsets to those page-absolute values.
Retain the existing centerline plus bounding_box conversion only for legacy
crop-local data when that contract is guaranteed, so page coordinates are not
offset twice.
Comment on lines
+733
to
+746
| # --- Overlay interpolated lines (dashed, same stave color but lighter) --- | ||
| for interp in result.interpolated_lines: | ||
| if interp.stave_id not in stave_colors: | ||
| continue | ||
| if not interp.y_values: | ||
| continue | ||
|
|
||
| xs = np.arange(interp.x_start, interp.x_end + 1) | ||
| ys = np.array(interp.y_values) | ||
| mask = (xs >= 0) & (xs < canvas_w) & (ys >= 0) & (ys < canvas_h) | ||
| xs = xs[mask].astype(np.int32) | ||
| ys = ys[mask].astype(np.int32) | ||
| if len(xs) < 2: | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Interpolated overlays mix crop-local x with page-absolute y.
interp.y_values are page-absolute (built as ref_f.y_page_offset + y in interpolate_staves.py), but interp.x_start/x_end are copied straight from FitResult.x_start/x_end, which are crop-local — no x_page_offset is ever applied, here or upstream. On any page whose crops don't start at x=0 the dashed lines are drawn at the wrong x, and the xs < canvas_w mask silently drops them once the offset is large.
The clean fix belongs upstream in interpolate_staves.py: store x_start/x_end as int(f.x_page_offset + f.x_start) / int(f.x_page_offset + f.x_end) so the InterpolatedLine docstring ("page-absolute") actually holds, which also makes the _reindex_stave_lines y-comparison and any JSON export consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/group_staves.py` around lines 733 - 746, Update the
interpolated-line construction in interpolate_staves.py to store page-absolute x
coordinates by adding each fit’s x_page_offset to f.x_start and f.x_end,
matching the existing page-absolute y_values behavior. Preserve integer
conversion and ensure the resulting InterpolatedLine fields,
_reindex_stave_lines comparisons, JSON export, and overlay logic use the
corrected coordinates.
Comment on lines
+106
to
+136
| def _interpolate_between( | ||
| above_fit: FitResult, | ||
| below_fit: FitResult, | ||
| x_start: int, | ||
| x_end: int, | ||
| y_target: float, | ||
| ) -> list[float]: | ||
| """Synthesise y-values for a missing line between two detected neighbours. | ||
|
|
||
| At each x, reads the y-value from the line above and below (clamping to | ||
| their endpoints if x falls outside their range), then linearly interpolates | ||
| to y_target based on its fractional position between the two neighbours. | ||
|
|
||
| Returns page-absolute y-values (y_page_offset already included). | ||
| """ | ||
|
|
||
| def y_at_x(fit: FitResult, x: int) -> float: | ||
| idx = x - fit.x_start | ||
| idx = max(0, min(idx, len(fit.y_values) - 1)) | ||
| return fit.y_page_offset + fit.y_values[idx] | ||
|
|
||
| ys: list[float] = [] | ||
| for x in range(x_start, x_end + 1): | ||
| ya = y_at_x(above_fit, x) | ||
| yb = y_at_x(below_fit, x) | ||
| if yb > ya: | ||
| t = max(0.0, min(1.0, (y_target - ya) / (yb - ya))) | ||
| ys.append(ya + t * (yb - ya)) | ||
| else: | ||
| ys.append((ya + yb) / 2.0) # degenerate: lines crossed or identical | ||
| return ys |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether FitResult.x_start/x_end are crop-local or page-absolute,
# and how x_page_offset is intended to be applied downstream.
rg -n -B2 -A15 'class FitResult' staff-finding/scripts/fit_centerline.py
rg -n 'x_page_offset|x_start\s*=|x_end\s*=' staff-finding/scripts/fit_centerline.py staff-finding/scripts/run_page.py staff-finding/scripts/group_staves.pyRepository: DDMAL/mothra
Length of output: 2418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the interpolation logic around the cited lines.
sed -n '240,360p' staff-finding/scripts/interpolate_staves.py | cat -n
# Also inspect the FitResult definition and any helper that uses x_page_offset nearby.
sed -n '78,130p' staff-finding/scripts/fit_centerline.py | cat -nRepository: DDMAL/mothra
Length of output: 8567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the InterpolatedLine definition and downstream consumers.
sed -n '1,120p' staff-finding/scripts/interpolate_staves.py | cat -n
rg -n 'InterpolatedLine|x_page_offset|x_start|x_end' staff-finding/scripts -g '!*.pyc'Repository: DDMAL/mothra
Length of output: 15842
Add x_page_offset when emitting InterpolatedLine x-ranges
FitResult.x_start/x_end are crop-local, but InterpolatedLine is treated as page-absolute. Both branches here pass raw crop-local x values into InterpolatedLine (min(above_f.x_start, below_f.x_start) / ref_f.x_start), so interpolated lines from crops with different x_page_offsets will be shifted. Add the offset before constructing the synthesised line.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/interpolate_staves.py` around lines 106 - 136, The
interpolated-line construction paths must convert crop-local x coordinates to
page-absolute coordinates. Locate both branches that create InterpolatedLine,
including the range using above_f.x_start/below_f.x_start and the branch using
ref_f.x_start, and add the corresponding x_page_offset before passing x-range
values; preserve the existing y interpolation behavior.
Comment on lines
+15
to
+17
| spec = importlib.util.spec_from_file_location( | ||
| "run_page_partial", "/home/claude/run_page.py" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Load run_pageOG.py through a repository-relative path.
Line 16 hard-codes a local path to run_page.py; this makes the test fail on other checkouts and means test_run_pageOG.py does not exercise the OG driver. Resolve staff-finding/scripts/run_pageOG.py relative to __file__ instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/script_tests/test_run_pageOG.py` around lines 15 - 17,
Update the import setup in test_run_pageOG.py to resolve
staff-finding/scripts/run_pageOG.py from the test file’s __file__ location,
replacing the hard-coded /home/claude/run_page.py path. Ensure the loaded module
targets the OG driver while remaining valid across repository checkouts.
Comment on lines
+97
to
+110
| def remap_label_file(src: Path, dst: Path) -> None: | ||
| """Copy a YOLO .txt label, remapping SOURCE_CLASS → TARGET_CLASS.""" | ||
| lines: list[str] = [] | ||
| with src.open() as fh: | ||
| for raw in fh: | ||
| parts = raw.strip().split() | ||
| if not parts: | ||
| continue | ||
| cls = int(parts[0]) | ||
| if cls == SOURCE_CLASS: | ||
| cls = TARGET_CLASS | ||
| lines.append(" ".join([str(cls)] + parts[1:])) | ||
| dst.parent.mkdir(parents=True, exist_ok=True) | ||
| dst.write_text("\n".join(lines) + "\n") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Non-staffline classes are copied into a single-class dataset.
Only class 2 is remapped; lines with any other class id are written through unchanged. Since the source TXTs come from the multi-class model (run_inference.py maps ids 0/1/2), the produced labels contain original class-0 boxes now indistinguishable from stafflines, plus class-1 boxes that violate the nc: 1 declared in the YAML. Filter to SOURCE_CLASS instead of remapping in place.
🐛 Proposed fix
cls = int(parts[0])
- if cls == SOURCE_CLASS:
- cls = TARGET_CLASS
- lines.append(" ".join([str(cls)] + parts[1:]))
+ if cls != SOURCE_CLASS:
+ continue # drop non-staffline annotations
+ lines.append(" ".join([str(TARGET_CLASS), *parts[1:]]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def remap_label_file(src: Path, dst: Path) -> None: | |
| """Copy a YOLO .txt label, remapping SOURCE_CLASS → TARGET_CLASS.""" | |
| lines: list[str] = [] | |
| with src.open() as fh: | |
| for raw in fh: | |
| parts = raw.strip().split() | |
| if not parts: | |
| continue | |
| cls = int(parts[0]) | |
| if cls == SOURCE_CLASS: | |
| cls = TARGET_CLASS | |
| lines.append(" ".join([str(cls)] + parts[1:])) | |
| dst.parent.mkdir(parents=True, exist_ok=True) | |
| dst.write_text("\n".join(lines) + "\n") | |
| def remap_label_file(src: Path, dst: Path) -> None: | |
| """Copy a YOLO .txt label, remapping SOURCE_CLASS → TARGET_CLASS.""" | |
| lines: list[str] = [] | |
| with src.open() as fh: | |
| for raw in fh: | |
| parts = raw.strip().split() | |
| if not parts: | |
| continue | |
| cls = int(parts[0]) | |
| if cls != SOURCE_CLASS: | |
| continue # drop non-staffline annotations | |
| lines.append(" ".join([str(TARGET_CLASS), *parts[1:]])) | |
| dst.parent.mkdir(parents=True, exist_ok=True) | |
| dst.write_text("\n".join(lines) + "\n") |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 108-108: Consider [str(cls), *parts[1:]] instead of concatenation
Replace with [str(cls), *parts[1:]]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/train_staffline_detector.py` around lines 97 - 110,
Update remap_label_file to discard every label whose class is not SOURCE_CLASS,
and only write retained SOURCE_CLASS entries using TARGET_CLASS. Preserve the
existing handling of blank lines and label coordinates so the generated
single-class dataset contains no class-0 or class-1 boxes.
…R export, and eval interpolate_staves.py never applied x_page_offset when constructing InterpolatedLine x-ranges (Trigger A and Trigger B), despite the dataclass docstring promising page-absolute coordinates - lines from crops with different offsets landed at the wrong x. Also fixes a latent index misalignment between centers and fit_pairs_sorted if any center were ever None. shared_utils.py write_jsomr had the same defect: it wrote fit.x_start, x_end, and y_values verbatim without the fit's x_page_offset/y_page_offset, currently masked because every caller pre-zeros the offsets, but a latent bug for any future runner using non-zero offsets. eval_page.py's page-y/page-x helpers added bounding_box offsets to values that are sometimes already page-absolute (the new centerline_page block from run_page.py), double-counting the offset. Now prefers centerline_page when present and falls back to the legacy crop-local conversion otherwise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ilter component_filter.py's companion-retention logic only checked that a candidate's x-range didn't overlap the winner's, unlike the merge-clustering path which also requires y-center proximity. A non-overlapping fragment of an adjacent (different) staffline could score above COMPANION_SCORE_FLOOR and get folded into the winner's pixel set, contaminating the fit. Mirrors the existing merge y-threshold guard. Also documents companion_labels in the result docstring and adds strict= to the coordinate zips. train_staffline_detector.py's remap_label_file only remapped SOURCE_CLASS labels and passed every other class through unchanged, so class-0/class-1 boxes from the multi-class source labels ended up in the single-class (nc: 1) dataset, indistinguishable from real stafflines. Now filters to SOURCE_CLASS only. Also chains the ultralytics ImportError for a cleaner traceback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_print_aggregate_summary filtered out the string "nan" but not the actual
float("nan") values that evaluate() can return (e.g. split_ratio,
mean_y_mae_px), so a single page without matches poisoned the mean/std/
min/max for its whole variant.
--staffline-class defaulted to 0, so argparse always supplied a value and
a present manifest column always won regardless of the flag, contradicting
its own help text ("overrides manifest column if set"). Now defaults to
None so an explicitly-passed flag can actually override the manifest.
Also aligns the default staffline class with the "staves" YOLO class (2)
used elsewhere in the pipeline, rather than 0.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gp_fit() returned a flat mean-y fallback (with inf std) whenever GaussianProcessRegressor.fit() raised, and the runner treated that as a normal fit result - a numerical or configuration failure would silently pollute stave grouping and exported predictions. gp_fit() now returns empty y_pred/y_std with meta["error"] set on failure (both the no-coords case and the fit-exception case), and run_gp_page.py checks for that error and excludes the box from grouping instead of building a fit from it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…by-one group_staves.py's stave-grouping diagnostic draws onto an RGB canvas (matplotlib imshow + a later RGB2BGR conversion before cv2.imwrite), but the stroke colors were built via reversed(color_rgb), swapping R and B so every drawn line mismatched its legend patch. Drops the reversal for both the detected and interpolated strokes. dp_tracer.py's trace-range clamping only lower-clamped x_start and only upper-clamped x_end, so a fully out-of-bounds range produced empty xs/ys arrays that crash the runner on xs[0]. Now clamps both endpoints and returns a single-point degenerate result for an inverted range instead of an empty one. periodicity_detector.py's upper-tooth band-row bound (r_up_hi) subtracted an extra 1 versus the mirrored lower-tooth formula, excluding the last valid row near the page boundary from the comb cost. Also removes the now-unused y_up_lo/y_up_hi clamp variables. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ation The refinement-acceptance gate compared refined_residuals (computed over ~N sliding-window medians) against abs_residuals (computed over every kept pixel). Window medians are structurally much closer to any curve than raw pixels, so the gate accepted refinement almost unconditionally, and residual_mean/residual_max silently switched population whenever refinement fired - making them incomparable across boxes in the summary CSV / JSOMR output. _trace_line now also returns the boolean mask of every raw pixel that fell in-band during the trace. The gate scores both the original and refined coefficients against that same in-band pixel set, and reports residuals from that set when refinement is accepted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every script_tests file hardcoded sys.path.insert(0, "/home/claude"), breaking on any other checkout. test_run_pageOG.py had it worse: it loaded "/home/claude/run_page.py" via importlib regardless, so it never actually exercised run_pageOG.py despite its name. All six files now resolve the scripts directory relative to __file__; test_run_pageOG.py loads run_pageOG.py specifically. Verified all six run standalone (mothrav8 env). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
*.pt was added after !models/**/*.pt, so the later broad pattern won and re-ignored everything under models/, defeating the intended exception. Reordered so the negation is last. staff-finding/e2e_tests was still listed as ignored despite this PR intentionally committing e2e regression fixtures under it (81 tracked files) - dropped the now-contradictory rule. Also removed a duplicate .claude/ entry introduced while merging gitignore changes from main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
STATUS.md, experiments/README.md, and implicit_neural/NOTES.md still
described interpolate_staves.py as a stub/placeholder and implicit_neural/
periodicity as "planned" - all three are implemented in this PR (the
former is just off by default pending validation). Updated the status
claims, the known-issues and key-files tables, and added run examples for
the two runners so the docs match what actually ships.
Also fixes two typos ("Downstrem", "at they go") and adds language hints
to three fenced code blocks flagged by markdownlint.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kyrieb-ekat
force-pushed
the
kyrie/staff-finding
branch
from
July 27, 2026 23:38
59ac380 to
fa7c333
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
staff-finding/scripts/component_filter.py (1)
280-299: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
discarded/score_breakdown["kept"]no longer match what's actually kept once companions are retained.All non-winning survivors are unconditionally recorded as
discarded(reason"not_top_scoring") andscore_breakdown[...]["kept"]is only everTrueforwinner_label— but the companion-retention pass below (lines 337-401) can fold some of these same survivors into the activecoords/mask. Downstream consumers ofresult.discarded(e.g.run_page.py'sn_discardedsummary column) will over-count truly-discarded components whenever companions are retained, since companion labels appear in bothdiscardedandcompanion_labels/coordssimultaneously.Consider excluding companion labels from
discarded, or clearly documenting thatdiscarded/keptreflect only the no-merge single-winner view and may not match the active output when companions are present.Also applies to: 422-425
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/component_filter.py` around lines 280 - 299, Update the survivor bookkeeping so retained companions are not reported as discarded. In the companion-retention flow, use the established companion-label collection to exclude those labels from discarded entries and mark their score_breakdown entries as kept alongside winner_label. Ensure result.discarded and score_breakdown reflect the final active coords/mask output rather than the intermediate single-winner view.
🧹 Nitpick comments (9)
staff-finding/scripts/group_staves.py (1)
530-573: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace Unicode minus signs in the docstring/comments.
Ruff flags
−(U+2212) on lines 532, 543, 550, 551 and 573 (RUF002/RUF003); use plain-so the lint run stays clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/group_staves.py` around lines 530 - 573, Replace every Unicode minus sign (U+2212) in the docstring and comments surrounding the stave classification and intra_count calculation with the ASCII hyphen-minus (-), including references to detected_line_count, mode_n, and the intra_count assignment in the affected function. Preserve the wording and logic otherwise.Source: Linters/SAST tools
staff-finding/experiments/gp_centerlines/gp_fitter.py (1)
145-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
int()casts.
len()already returnsint; Ruff flags both (RUF046).♻️ Proposed cleanup
- "n_obs_cols": int(len(unique_xs)), - "n_obs_pixels": int(len(coords)), + "n_obs_cols": len(unique_xs), + "n_obs_pixels": len(coords),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/gp_centerlines/gp_fitter.py` around lines 145 - 148, Remove the redundant int() wrappers from the len(unique_xs) and len(coords) values in the result mapping, while preserving the existing keys and values.Source: Linters/SAST tools
staff-finding/experiments/gp_centerlines/run_gp_page.py (2)
197-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAppend the uncertainty flag rather than replacing the list.
fit.flags = [...]discards anything set during construction. Harmless today, but it will silently drop flags the moment one is added upstream.♻️ Proposed change
- fit.flags = [f"mean_uncertainty_px:{mean_std:.2f}"] + fit.flags = [*fit.flags, f"mean_uncertainty_px:{mean_std:.2f}"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/gp_centerlines/run_gp_page.py` around lines 197 - 199, Update the uncertainty handling near fit.flags so the mean_uncertainty_px flag is appended to the existing flags collection rather than replacing it. Preserve all flags initialized during fit construction while retaining the current mean_std calculation and formatting.
134-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnify the box convention and silence the unused unpacking.
The
empty_cropbranch records the unpadded box while every other branch recordsactual_box, soboxesmixes two conventions in the exported JSOMR.lrx_a/lry_aare also unused (RUF059).♻️ Proposed cleanup
- ulx_a, uly_a, lrx_a, lry_a = actual_box + ulx_a, uly_a = actual_box[0], actual_box[1] if crop.size == 0: fit_results.append(ExperimentFitResult(flags=["empty_crop"])) - boxes.append((ulx, uly, lrx, lry)) + boxes.append(actual_box) continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/gp_centerlines/run_gp_page.py` around lines 134 - 139, Update the empty-crop branch in the surrounding box-processing flow to append actual_box’s coordinates, matching the convention used by non-empty crops. Remove the unused lrx_a and lry_a unpacking while retaining the coordinates required by the branch.Source: Linters/SAST tools
staff-finding/scripts/script_tests/test_fit_centerline.py (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
/tmp/...pngdiagnostic paths (4 occurrences).Flagged by both Ruff (S108) and ast-grep as CWE-377; not exploitable here since it's local test-debug output, but it's not portable (Windows has no
/tmp) and risks collisions under parallel test execution.♻️ Proposed fix — use `tempfile`
+import tempfile + +_TMP_DIR = Path(tempfile.gettempdir()) + def test_clean_line_fit(): ... - filter_result, scale_unit=4.0, crop=crop, save_path=Path("/tmp/fit_clean.png") + filter_result, scale_unit=4.0, crop=crop, save_path=_TMP_DIR / "fit_clean.png"Apply the same pattern to the other three
save_path=Path("/tmp/...")call sites (lines 54, 85, 135).Also applies to: 53-55, 84-86, 131-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/script_tests/test_fit_centerline.py` around lines 30 - 32, Replace all four hardcoded save_path values in test_fit_centerline.py, including the fit_centerline call and the other highlighted call sites, with paths created through tempfile so they are portable and unique during parallel tests. Preserve the existing diagnostic filenames or output behavior as appropriate while ensuring each temporary path is safely managed.Source: Linters/SAST tools
staff-finding/experiments/implicit_neural/implicit_neural_fitter.py (1)
178-190: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull-page tensor rebuilt on every call — redundant per-line cost.
gray_tensoris a full(1,1,H,W)float32 conversion of the entire page, recomputed on eachimplicit_neural_fit()call. Perimplicit_neural/NOTES.mdin this PR, this runs ~86 times per page (once per staffline), so the same page-sized conversion is redone dozens of times even though each call only samples a narrow y-band. Consider accepting an optional pre-convertedgray_tensorparameter (built once by the caller) or memoizing it, since the underlying image doesn't change between calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/implicit_neural/implicit_neural_fitter.py` around lines 178 - 190, The implicit_neural_fit flow rebuilds the full-page gray_tensor on every staffline call. Add an optional preconverted gray_tensor parameter to implicit_neural_fit, reuse it when supplied, and keep the existing conversion as a fallback when it is absent; update the page-level caller to construct the tensor once and pass it to each call.staff-finding/scripts/component_filter.py (2)
337-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo dedicated test coverage for companion retention.
This is a substantial new behaviour (score-floor + y-proximity + x-non-overlap gating that folds extra components into
coords/mask/companion_labels), but the reviewed test files (test_merge.py,test_merge_step.py) only exercise merge-cluster behaviour, not companion retention specifically. Consider adding a test that constructs a crop with a plausible companion fragment (passing the score floor, within the y-threshold, non-overlapping x-range) and asserts it appears incompanion_labelsand contributes tocoords.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/component_filter.py` around lines 337 - 401, The companion-retention logic in the survivor-processing loop lacks dedicated coverage. Add a focused test using a crop containing a companion fragment that passes the score floor, y-proximity, and x non-overlap checks, then assert the fragment’s label is included in companion_labels and its pixels are included in the resulting coords, covering the relevant merge/no-merge path exposed by the existing tests.
301-304: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMissing
strict=Trueon thiszip(), unlike the newer companion zips.
no_merge_coords = list(zip(xs.tolist(), ys.tolist()))doesn't usestrict=True, while the merge/companion zips added in this same diff (lines 335, 380) do.xs/ysare always same-length here (both fromnp.whereon one mask), so this is purely a consistency nit, but addingstrict=Truecosts nothing and matches the pattern established elsewhere in this function.♻️ Proposed fix
- no_merge_coords = list(zip(xs.tolist(), ys.tolist())) + no_merge_coords = list(zip(xs.tolist(), ys.tolist(), strict=True))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/scripts/component_filter.py` around lines 301 - 304, Update the no_merge_coords construction in the no-merge mask setup to call zip with strict=True, matching the companion zip calls elsewhere in the function while preserving the existing coordinate output.Source: Linters/SAST tools
staff-finding/experiments/dp_tracing/run_dp_page.py (1)
76-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused loop variable
idx.
idxfromenumerate(stafflines)is never used inside the loop body (flagged by Ruff B007). Rename to_idxor dropenumerate.♻️ Proposed fix
- for idx, det in enumerate(stafflines): + for det in stafflines:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff-finding/experiments/dp_tracing/run_dp_page.py` around lines 76 - 111, Remove the unused idx variable in the stafflines iteration by replacing enumerate(stafflines) with direct iteration over stafflines, or rename it to _idx if enumeration is required; preserve the existing loop body behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@staff-finding/experiments/periodicity/periodicity_detector.py`:
- Around line 198-220: Extract the full-page smoothing and darkness calculation
from periodicity_trace into a reusable compute_dark_field helper. In
run_periodicity_page.py, call compute_dark_field once before the staffline
iteration and pass the resulting dark field through each periodicity_trace call
via its dark parameter; update periodicity_trace to reuse the supplied field
while preserving existing behavior when no field is provided.
In `@staff-finding/scripts/group_staves.py`:
- Around line 149-152: Update the `group_staves` docstring for
`interpolation_max_gap` to describe the actual default resolution through
`_compute_interpolation_max_gap(all_gaps, cut_threshold)` when gaps are present,
while noting the `cut_threshold * INTERPOLATION_GAP_MULTIPLIER` fallback only
applies when no gaps exist.
---
Outside diff comments:
In `@staff-finding/scripts/component_filter.py`:
- Around line 280-299: Update the survivor bookkeeping so retained companions
are not reported as discarded. In the companion-retention flow, use the
established companion-label collection to exclude those labels from discarded
entries and mark their score_breakdown entries as kept alongside winner_label.
Ensure result.discarded and score_breakdown reflect the final active coords/mask
output rather than the intermediate single-winner view.
---
Nitpick comments:
In `@staff-finding/experiments/dp_tracing/run_dp_page.py`:
- Around line 76-111: Remove the unused idx variable in the stafflines iteration
by replacing enumerate(stafflines) with direct iteration over stafflines, or
rename it to _idx if enumeration is required; preserve the existing loop body
behavior.
In `@staff-finding/experiments/gp_centerlines/gp_fitter.py`:
- Around line 145-148: Remove the redundant int() wrappers from the
len(unique_xs) and len(coords) values in the result mapping, while preserving
the existing keys and values.
In `@staff-finding/experiments/gp_centerlines/run_gp_page.py`:
- Around line 197-199: Update the uncertainty handling near fit.flags so the
mean_uncertainty_px flag is appended to the existing flags collection rather
than replacing it. Preserve all flags initialized during fit construction while
retaining the current mean_std calculation and formatting.
- Around line 134-139: Update the empty-crop branch in the surrounding
box-processing flow to append actual_box’s coordinates, matching the convention
used by non-empty crops. Remove the unused lrx_a and lry_a unpacking while
retaining the coordinates required by the branch.
In `@staff-finding/experiments/implicit_neural/implicit_neural_fitter.py`:
- Around line 178-190: The implicit_neural_fit flow rebuilds the full-page
gray_tensor on every staffline call. Add an optional preconverted gray_tensor
parameter to implicit_neural_fit, reuse it when supplied, and keep the existing
conversion as a fallback when it is absent; update the page-level caller to
construct the tensor once and pass it to each call.
In `@staff-finding/scripts/component_filter.py`:
- Around line 337-401: The companion-retention logic in the survivor-processing
loop lacks dedicated coverage. Add a focused test using a crop containing a
companion fragment that passes the score floor, y-proximity, and x non-overlap
checks, then assert the fragment’s label is included in companion_labels and its
pixels are included in the resulting coords, covering the relevant
merge/no-merge path exposed by the existing tests.
- Around line 301-304: Update the no_merge_coords construction in the no-merge
mask setup to call zip with strict=True, matching the companion zip calls
elsewhere in the function while preserving the existing coordinate output.
In `@staff-finding/scripts/group_staves.py`:
- Around line 530-573: Replace every Unicode minus sign (U+2212) in the
docstring and comments surrounding the stave classification and intra_count
calculation with the ASCII hyphen-minus (-), including references to
detected_line_count, mode_n, and the intra_count assignment in the affected
function. Preserve the wording and logic otherwise.
In `@staff-finding/scripts/script_tests/test_fit_centerline.py`:
- Around line 30-32: Replace all four hardcoded save_path values in
test_fit_centerline.py, including the fit_centerline call and the other
highlighted call sites, with paths created through tempfile so they are portable
and unique during parallel tests. Preserve the existing diagnostic filenames or
output behavior as appropriate while ensuring each temporary path is safely
managed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d8acfa8-8952-4919-8260-427551e0d090
⛔ Files ignored due to path filters (54)
data/First_round_of_corrections-zips/F-PnLatin15181_169v_bigger_annotations.zipis excluded by!**/*.zipstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stave_grouping.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0000.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0000_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0001.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0001_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0002.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0002_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0003.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0003_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0004.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0004_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0005.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0005_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0100.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0100_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0102.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0102_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0104.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0104_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0107.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0107_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0108.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0108_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0109.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0109_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0110.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/box_0110_fit.pngis excluded by!**/*.pngstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/summary.csvis excluded by!**/*.csvstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-001/Ordo_virtutum-001_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-002/Ordo_virtutum-002_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-003/Ordo_virtutum-003_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-004/Ordo_virtutum-004_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-005/Ordo_virtutum-005_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-006/Ordo_virtutum-006_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-007/Ordo_virtutum-007_predicted.jpgis excluded by!**/*.jpgstaff-finding/e2e_tests/29may/Gent15_17_left/run_page/GentAnt1475_0017_leftcrop_no_bgr/summary.csvis excluded by!**/*.csvstaff-finding/e2e_tests/29may/Gent15_17_right/eval_batch_results.csvis excluded by!**/*.csvstaff-finding/e2e_tests/29may/Gent15_17_right/eval_manifest.csvis excluded by!**/*.csvstaff-finding/e2e_tests/29may/Gent15_17_right/eval_results.csvis excluded by!**/*.csvstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/GentAnt1475_0017_AC_rightcrop_no_bgr/summary.csvis excluded by!**/*.csvstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/otsu_no_bgr/summary.csvis excluded by!**/*.csvstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/sauvola_no_bgr/GentAnt1475_0017_AC_rightcrop_no_bgr/summary.csvis excluded by!**/*.csvstaff-finding/e2e_tests/pitch_finding_sample/GentAnt1475_0017_AC_rightcrop_no_bgr/GentAnt1475_0017_AC_rightcrop_stave_grouping.pngis excluded by!**/*.pngstaff-finding/e2e_tests/pitch_finding_sample/GentAnt1475_0017_AC_rightcrop_no_bgr/GentAnt1475_0017_AC_rightcrop_stave_grouping_hq.pngis excluded by!**/*.pngstaff-finding/e2e_tests/pitch_finding_sample/GentAnt1475_0017_AC_rightcrop_no_bgr/summary.csvis excluded by!**/*.csvstaff-finding/image-sets/gent/Gent15_17_left_screenshot.pngis excluded by!**/*.pngstaff-finding/image-sets/gent/Gent15_17_right_screenshot.pngis excluded by!**/*.pngstaff-finding/image-sets/gent/left/GentAnt1475_0017_leftcrop.jpgis excluded by!**/*.jpgstaff-finding/image-sets/gent/right/GentAnt1475_0017_AC_rightcrop.jpgis excluded by!**/*.jpgstaff-finding/image-sets/gent/right/inference/GentAnt1475_0017_AC_rightcrop/GentAnt1475_0017_AC_rightcrop_predicted.jpgis excluded by!**/*.jpgstaff-finding/scripts/__pycache__/bgr_adapter.cpython-312.pycis excluded by!**/*.pycstaff-finding/scripts/__pycache__/yolo_io.cpython-312.pycis excluded by!**/*.pyc
📒 Files selected for processing (88)
.gitignoreREADME.mddata/First_round_of_corrections-zips/CH-Fco Ms.2 108v_annotations/CH-Fco Ms.2 108v.jsondata/First_round_of_corrections-zips/CH-Fco Ms.2 108v_annotations/CH-Fco Ms.2 108v.txtdata/fast_pairs/F-PnLatin15181_169v_biggerCORRECTED.jsondata/fast_pairs/F-PnLatin15181_169v_biggerCORRECTED.txtscripts/run_inference.pystaff-finding/README.mdstaff-finding/dox/IMPLEMENTATION_AUDIT.mdstaff-finding/dox/IMPLEMENTATION_NOTES.mdstaff-finding/dox/PITCH_FINDING_NOTES.mdstaff-finding/dox/STATUS.mdstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000_model-predicted.txtstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/Ordo_virtutum-000_stafflines.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txtstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-001/Ordo_virtutum-001.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-002/Ordo_virtutum-002.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-003/Ordo_virtutum-003.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-004/Ordo_virtutum-004.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-005/Ordo_virtutum-005.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-006/Ordo_virtutum-006.jsonstaff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-007/Ordo_virtutum-007.jsonstaff-finding/e2e_tests/28may_stave-fulldata/all_predictions.jsonstaff-finding/e2e_tests/29may/Gent15_17_left/Gent15_17_left.jsonstaff-finding/e2e_tests/29may/Gent15_17_left/run_page/GentAnt1475_0017_leftcrop_no_bgr/GentAnt1475_0017_leftcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_left/run_page/GentAnt1475_0017_leftcrop_no_bgr/stave_grouping_report.txtstaff-finding/e2e_tests/29may/Gent15_17_right/Gent15_17_right_corrected.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/Gent15_17_right_corrected.txtstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/GentAnt1475_0017_AC_rightcrop_no_bgr/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/GentAnt1475_0017_AC_rightcrop_no_bgr/stave_grouping_report.txtstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/dp_tracing/GentAnt1475_0017_AC_rightcrop_dp/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/gp_centerlines/GentAnt1475_0017_AC_rightcrop_gp/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/gp_centerlines_shape_metrics/GentAnt1475_0017_AC_rightcrop_gp/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/implicit_neural/GentAnt1475_0017_AC_rightcrop_implicit_neural/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/implicit_neural_v2_green/GentAnt1475_0017_AC_rightcrop_implicit_neural/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/implicit_neural_v3_interp/GentAnt1475_0017_AC_rightcrop_implicit_neural/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/implicit_neural_v4_periodicity/GentAnt1475_0017_AC_rightcrop_implicit_neural/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/otsu_no_bgr/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/otsu_no_bgr/stave_grouping_report.txtstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/periodicity/GentAnt1475_0017_AC_rightcrop_periodicity/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/periodicity_v2/GentAnt1475_0017_AC_rightcrop_periodicity/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/periodicity_v3/GentAnt1475_0017_AC_rightcrop_periodicity/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/sauvola_no_bgr/GentAnt1475_0017_AC_rightcrop_no_bgr/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/29may/Gent15_17_right/run_page/sauvola_no_bgr/GentAnt1475_0017_AC_rightcrop_no_bgr/stave_grouping_report.txtstaff-finding/e2e_tests/pitch_finding_sample/GentAnt1475_0017_AC_rightcrop_no_bgr/GentAnt1475_0017_AC_rightcrop_stafflines.jsonstaff-finding/e2e_tests/pitch_finding_sample/GentAnt1475_0017_AC_rightcrop_no_bgr/stave_grouping_report.txtstaff-finding/experiments/README.mdstaff-finding/experiments/dp_tracing/dp_tracer.pystaff-finding/experiments/dp_tracing/run_dp_page.pystaff-finding/experiments/gp_centerlines/gp_fitter.pystaff-finding/experiments/gp_centerlines/run_gp_page.pystaff-finding/experiments/heatmap_regression/NOTES.mdstaff-finding/experiments/implicit_neural/NOTES.mdstaff-finding/experiments/implicit_neural/implicit_neural_fitter.pystaff-finding/experiments/implicit_neural/run_implicit_neural_page.pystaff-finding/experiments/periodicity/NOTES.mdstaff-finding/experiments/periodicity/periodicity_detector.pystaff-finding/experiments/periodicity/run_periodicity_page.pystaff-finding/experiments/shared_utils.pystaff-finding/image-sets/gent/all_predictions.jsonstaff-finding/image-sets/gent/right/Gent15_17_right.jsonstaff-finding/image-sets/gent/right/inference/GentAnt1475_0017_AC_rightcrop/GentAnt1475_0017_AC_rightcrop.jsonstaff-finding/image-sets/gent/right/inference/all_predictions.jsonstaff-finding/image-sets/gent/right/inference/corrected/GentAnt1475_0017_AC_rightcrop.jsonstaff-finding/image-sets/gent/right/inference/corrected/GentAnt1475_0017_AC_rightcrop.txtstaff-finding/image-sets/ordo_virt_000/Ordo_virtutum-000.jsonstaff-finding/image-sets/ordo_virt_000/Ordo_virtutum-000.txtstaff-finding/scripts/bgr_adapter.pystaff-finding/scripts/component_filter.pystaff-finding/scripts/eval_batch.pystaff-finding/scripts/eval_page.pystaff-finding/scripts/fit_centerline.pystaff-finding/scripts/group_staves.pystaff-finding/scripts/interpolate_staves.pystaff-finding/scripts/run_page.pystaff-finding/scripts/run_pageOG.pystaff-finding/scripts/script_tests/c-filter_test.pystaff-finding/scripts/script_tests/test_fit_centerline.pystaff-finding/scripts/script_tests/test_group_staves.pystaff-finding/scripts/script_tests/test_merge.pystaff-finding/scripts/script_tests/test_merge_step.pystaff-finding/scripts/script_tests/test_run_page.pystaff-finding/scripts/script_tests/test_run_pageOG.pystaff-finding/scripts/script_tests/test_sauvola.pystaff-finding/scripts/test_group_staves.pystaff-finding/scripts/train_staffline_detector.pystaff-finding/scripts/yolo_io.py
🚧 Files skipped from review as they are similar to previous changes (33)
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000_model-predicted.txt
- staff-finding/image-sets/ordo_virt_000/Ordo_virtutum-000.txt
- staff-finding/e2e_tests/29may/Gent15_17_right/run_page/sauvola_no_bgr/GentAnt1475_0017_AC_rightcrop_no_bgr/stave_grouping_report.txt
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/run_page_results/Ordo_virtutum-000_no_bgr/stave_grouping_report.txt
- staff-finding/image-sets/gent/right/inference/GentAnt1475_0017_AC_rightcrop/GentAnt1475_0017_AC_rightcrop.json
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-005/Ordo_virtutum-005.json
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-004/Ordo_virtutum-004.json
- README.md
- staff-finding/e2e_tests/29may/Gent15_17_right/run_page/otsu_no_bgr/stave_grouping_report.txt
- staff-finding/scripts/script_tests/test_merge.py
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-001/Ordo_virtutum-001.json
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-007/Ordo_virtutum-007.json
- .gitignore
- staff-finding/image-sets/gent/right/inference/corrected/GentAnt1475_0017_AC_rightcrop.json
- staff-finding/image-sets/gent/right/Gent15_17_right.json
- staff-finding/e2e_tests/28may_stave-fulldata/all_predictions.json
- staff-finding/e2e_tests/29may/Gent15_17_right/run_page/GentAnt1475_0017_AC_rightcrop_no_bgr/stave_grouping_report.txt
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-000/Ordo_virtutum-000.json
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-003/Ordo_virtutum-003.json
- staff-finding/scripts/script_tests/test_sauvola.py
- staff-finding/image-sets/gent/right/inference/all_predictions.json
- staff-finding/scripts/script_tests/test_run_pageOG.py
- staff-finding/e2e_tests/28may_stave-fulldata/Ordo_virtutum-006/Ordo_virtutum-006.json
- staff-finding/scripts/test_group_staves.py
- staff-finding/experiments/shared_utils.py
- staff-finding/dox/IMPLEMENTATION_NOTES.md
- staff-finding/scripts/bgr_adapter.py
- staff-finding/scripts/script_tests/test_group_staves.py
- staff-finding/scripts/script_tests/c-filter_test.py
- data/fast_pairs/F-PnLatin15181_169v_biggerCORRECTED.txt
- staff-finding/scripts/run_page.py
- staff-finding/scripts/fit_centerline.py
- staff-finding/scripts/run_pageOG.py
Comment on lines
+198
to
+220
| # Optional Gaussian pre-blur along rows (axis=0) to suppress noise. | ||
| sigma = blur_sigma_multiplier * scale_unit | ||
| if sigma > 0.5: | ||
| smoothed = gaussian_filter1d(gray.astype(np.float32), sigma=sigma, axis=0) | ||
| else: | ||
| smoothed = gray.astype(np.float32) | ||
|
|
||
| # dark[y, x] = (255 - pixel) / 255 for the full page, so dark ink = high value. | ||
| # We keep the full page here so that comb teeth can reach rows outside the band. | ||
| dark = (255.0 - smoothed) / 255.0 # shape: (page_h, page_w) | ||
|
|
||
| # ----------------------------------------------------------------- | ||
| # Build comb cost for the search band. | ||
| # cost[r, c] = darkness(y_lo+r, x_start+c) | ||
| # + teeth_weight * darkness(y_lo+r - h_est, x_start+c) [if in bounds] | ||
| # + teeth_weight * darkness(y_lo+r + h_est, x_start+c) [if in bounds] | ||
| # + … for further teeth | ||
| # Then normalised to [0, 1] and inverted so low cost = dark = good. | ||
| # ----------------------------------------------------------------- | ||
|
|
||
| # Centre slice: band rows only, columns x_start..x_end. | ||
| # shape: (n_band, n_cols) | ||
| band_dark = dark[y_lo : y_hi + 1, x_start : x_end + 1] |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Full-page Gaussian blur + darkness field recomputed on every staffline call.
smoothed/dark depend only on gray and scale_unit (via blur_sigma_multiplier), both invariant across the whole page — yet run_periodicity_page.py's per-staffline loop calls periodicity_trace() once per detection, redoing this full-page Gaussian blur and normalization every time. For pages with dozens of stafflines this multiplies an expensive full-page operation dozens of times for no benefit.
♻️ Proposed fix — hoist the blur/dark computation out of the per-box call
+def compute_dark_field(
+ gray: np.ndarray, scale_unit: float, blur_sigma_multiplier: float = BLUR_SIGMA_MULTIPLIER
+) -> np.ndarray:
+ """Precompute the page-wide darkness field once per page."""
+ sigma = blur_sigma_multiplier * scale_unit
+ if sigma > 0.5:
+ smoothed = gaussian_filter1d(gray.astype(np.float32), sigma=sigma, axis=0)
+ else:
+ smoothed = gray.astype(np.float32)
+ return (255.0 - smoothed) / 255.0
+
+
def periodicity_trace(
gray: np.ndarray,
y_hint: float,
x_start: int,
x_end: int,
scale_unit: float,
h_est: float | None = None,
band_half_multiplier: float = BAND_HALF_MULTIPLIER,
max_step_px: int = MAX_STEP_PX,
blur_sigma_multiplier: float = BLUR_SIGMA_MULTIPLIER,
n_teeth: int = N_TEETH,
teeth_weight: float = TEETH_WEIGHT,
+ dark: np.ndarray | None = None,
) -> tuple[np.ndarray, np.ndarray]:
...
- sigma = blur_sigma_multiplier * scale_unit
- if sigma > 0.5:
- smoothed = gaussian_filter1d(gray.astype(np.float32), sigma=sigma, axis=0)
- else:
- smoothed = gray.astype(np.float32)
- dark = (255.0 - smoothed) / 255.0
+ if dark is None:
+ dark = compute_dark_field(gray, scale_unit, blur_sigma_multiplier)Then in run_periodicity_page.py, compute dark_field = compute_dark_field(gray, scale_unit, blur_sigma_multiplier) once before the for idx, det in enumerate(stafflines): loop and pass dark=dark_field into each periodicity_trace(...) call.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Optional Gaussian pre-blur along rows (axis=0) to suppress noise. | |
| sigma = blur_sigma_multiplier * scale_unit | |
| if sigma > 0.5: | |
| smoothed = gaussian_filter1d(gray.astype(np.float32), sigma=sigma, axis=0) | |
| else: | |
| smoothed = gray.astype(np.float32) | |
| # dark[y, x] = (255 - pixel) / 255 for the full page, so dark ink = high value. | |
| # We keep the full page here so that comb teeth can reach rows outside the band. | |
| dark = (255.0 - smoothed) / 255.0 # shape: (page_h, page_w) | |
| # ----------------------------------------------------------------- | |
| # Build comb cost for the search band. | |
| # cost[r, c] = darkness(y_lo+r, x_start+c) | |
| # + teeth_weight * darkness(y_lo+r - h_est, x_start+c) [if in bounds] | |
| # + teeth_weight * darkness(y_lo+r + h_est, x_start+c) [if in bounds] | |
| # + … for further teeth | |
| # Then normalised to [0, 1] and inverted so low cost = dark = good. | |
| # ----------------------------------------------------------------- | |
| # Centre slice: band rows only, columns x_start..x_end. | |
| # shape: (n_band, n_cols) | |
| band_dark = dark[y_lo : y_hi + 1, x_start : x_end + 1] | |
| # Optional Gaussian pre-blur along rows (axis=0) to suppress noise. | |
| if dark is None: | |
| dark = compute_dark_field(gray, scale_unit, blur_sigma_multiplier) | |
| # dark[y, x] = (255 - pixel) / 255 for the full page, so dark ink = high value. | |
| # We keep the full page here so that comb teeth can reach rows outside the band. | |
| # ----------------------------------------------------------------- | |
| # Build comb cost for the search band. | |
| # cost[r, c] = darkness(y_lo+r, x_start+c) | |
| # + teeth_weight * darkness(y_lo+r - h_est, x_start+c) [if in bounds] | |
| # + teeth_weight * darkness(y_lo+r + h_est, x_start+c) [if in bounds] | |
| # + … for further teeth | |
| # Then normalised to [0, 1] and inverted so low cost = dark = good. | |
| # ----------------------------------------------------------------- | |
| # Centre slice: band rows only, columns x_start..x_end. | |
| # shape: (n_band, n_cols) | |
| band_dark = dark[y_lo : y_hi + 1, x_start : x_end + 1] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/experiments/periodicity/periodicity_detector.py` around lines
198 - 220, Extract the full-page smoothing and darkness calculation from
periodicity_trace into a reusable compute_dark_field helper. In
run_periodicity_page.py, call compute_dark_field once before the staffline
iteration and pass the resulting dark field through each periodicity_trace call
via its dark parameter; update periodicity_trace to reuse the supplied field
while preserving existing behavior when no field is provided.
Comment on lines
+149
to
+152
| interpolation_max_gap: Upper gap bound (pixels) for the | ||
| interpolation trigger. Gaps above this are treated as | ||
| inter-stave spacing and are not filled. Defaults to | ||
| cut_threshold * INTERPOLATION_GAP_MULTIPLIER when None. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring default for interpolation_max_gap doesn't match behaviour.
group_staves always passes all_gaps=gaps, so interpolate_missing_lines resolves the bound via _compute_interpolation_max_gap(all_gaps, cut_threshold); the cut_threshold * INTERPOLATION_GAP_MULTIPLIER path only applies when there are no gaps at all.
📝 Proposed doc fix
interpolation_max_gap: Upper gap bound (pixels) for the
interpolation trigger. Gaps above this are treated as
- inter-stave spacing and are not filled. Defaults to
- cut_threshold * INTERPOLATION_GAP_MULTIPLIER when None.
+ inter-stave spacing and are not filled. When None it is
+ derived adaptively from the page gap distribution (see
+ interpolate_staves._compute_interpolation_max_gap), falling
+ back to cut_threshold * INTERPOLATION_GAP_MULTIPLIER when no
+ gaps are available.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| interpolation_max_gap: Upper gap bound (pixels) for the | |
| interpolation trigger. Gaps above this are treated as | |
| inter-stave spacing and are not filled. Defaults to | |
| cut_threshold * INTERPOLATION_GAP_MULTIPLIER when None. | |
| interpolation_max_gap: Upper gap bound (pixels) for the | |
| interpolation trigger. Gaps above this are treated as | |
| inter-stave spacing and are not filled. When None it is | |
| derived adaptively from the page gap distribution (see | |
| interpolate_staves._compute_interpolation_max_gap), falling | |
| back to cut_threshold * INTERPOLATION_GAP_MULTIPLIER when no | |
| gaps are available. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staff-finding/scripts/group_staves.py` around lines 149 - 152, Update the
`group_staves` docstring for `interpolation_max_gap` to describe the actual
default resolution through `_compute_interpolation_max_gap(all_gaps,
cut_threshold)` when gaps are present, while noting the `cut_threshold *
INTERPOLATION_GAP_MULTIPLIER` fallback only applies when no gaps exist.
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
1 participant
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.
Summary
Line-following refinement (
fit_centerline.py): when a bounding box contains multiple arching stafflines (page curvature), the initial Huber fit averages across them and produces high residuals. A new sliding-window trace walks left-to-right within a ±1.5h band from the box vertical center, isolating one line before refitting with a cubic polynomial. Handles both C-shaped arches and S-shaped parchment waves. Flagged asline_following_applied:deg3/deg2.Page-coordinate offset fix (
fit_centerline.py,run_page.py,group_staves.py):FitResultcoordinates are crop-local. Addsx_page_offset/y_page_offsetfields (populated from YOLO bounding box ulx/uly inrun_page.py). Fixes two bugs:Stave bounding boxes (
group_staves.py): red rectangles +Snlabels drawn around each stave group in the diagnostic PNG for visual QA.Test fix (
test_fit_centerline.py): fixed hardcoded/home/claudesys.path; addedtest_two_line_box_line_followingverifying the line-following trigger and refit quality.Test plan
python -m pytest staff-finding/scripts/script_tests/test_fit_centerline.py staff-finding/scripts/script_tests/test_group_staves.py -v— 15 tests passrun_page.pyon a real page; confirm grouping diagnostic shows colored centerlines at correct positions, red stave boxes, and a reasonable mode lines/stave (~4–5 for square notation)line_following_appliedflag appears in fit diagnostics for boxes with high initial residuals🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests