feat: integrate staff-finding staffline detection into the landing-page predict/encode pipeline - #53
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe change adds a packaged staffline pipeline with fitting, grouping, interpolation, fallback redetection, evaluation, training, landing-page persistence, experiment runners, CI integration, documentation, and annotation and end-to-end fixtures. ChangesStaff-finding pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 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.
…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
…less of has_annotation The independent has_annotation/has_text_alignment skip logic only assigned yolo_txt, img_arr, and ann_id inside the has_annotation is False branch. An image with an existing annotation but no text_alignment yet, exactly the case that logic exists to support, reached has_class(yolo_txt, ...) with those names unassigned: a crash on the first such image, or silently reused a previous iterations stale values otherwise, writing the wrong images pixel data under the current images id. img_arr now decodes unconditionally, since staffline detection needs it either way, and the has_annotation branch fetches the existing annotation id and yolo_txt a prior run already wrote instead of leaving them undefined. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
landing-page/scripts/tasks_predict.py (2)
135-143: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate cooperative cancellation into staffline processing.
The only cancellation check is before each image at Line 99.
run_staffline_detection()performs its detection loop before yielding events, so a cancellation request can remain unobserved during a long staffline pass. Pass a cancellation callback into the stage and calljob_store.check_cancelled()inside its detection loop.As per coding guidelines, long-running Celery tasks in
landing-page/scripts/tasks_*.pymust calljob_store.check_cancelled()inside their processing loops.🤖 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 `@landing-page/scripts/tasks_predict.py` around lines 135 - 143, Update run_staffline_detection and its call site in the image-processing flow to accept a cancellation callback, passing job_store.check_cancelled from the task. Invoke the callback inside the staffline detection loop so cancellation is observed during processing, while preserving the existing event publishing behavior.Source: Coding guidelines
135-143: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep staffline detection reachable when only text alignment exists.
When
has_text_alignmentis true buthas_annotationis false, the new YOLO annotation is written, then Lines 131-133continuebefore this block runs. YOLO may therefore produce stave-class boxes without anystaffline_detectionsrow being created. Gate only text-finding onhas_text_alignment; do not skip the staffline stage.As per coding guidelines,
landing-page/scripts/tasks_predict.pymust run staffline detection after YOLO produces stave-class boxes throughstaffline_stage.pyand persist the result instaffline_detections.🤖 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 `@landing-page/scripts/tasks_predict.py` around lines 135 - 143, Update the control flow in the prediction loop so the `continue` associated with `has_text_alignment` only skips text-finding, not staffline processing. Ensure the `has_class(yolo_txt, STAFFLINE_CLASS_ID)` branch still invokes `run_staffline_detection` and publishes its events when `has_text_alignment` is true and `has_annotation` is false, allowing `staffline_stage.py` results to be persisted in `staffline_detections`.Source: Coding guidelines
landing-page/scripts/auth_api.py (1)
229-244: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not treat rolled-back schema initialisation as successful.
Any caught DDL error rolls back every statement in
init_db()before returning normally, so startup can proceed with required tables and indexes absent. Serialize setup with an advisory lock, retry the complete transaction, or re-raise after cleanup.🤖 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 `@landing-page/scripts/auth_api.py` around lines 229 - 244, Update init_db’s exception handling so caught DDL errors cannot return as successful initialization after con.rollback(). Serialize schema setup with an advisory lock or retry the complete transaction, and if initialization still fails, re-raise the error after closing the cursor and releasing the connection; preserve cleanup in finally.
🤖 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 `@landing-page/scripts/tasks_predict.py`:
- Around line 100-114: Handle the race in the has_annotation branch of the
image-processing task: the later annotations query may return no row after
validation. Reuse the annotation row captured during validation when available,
or check fetchone() before unpacking and reclassify the image as unannotated
when it disappears, preserving the normal processing flow without raising
TypeError.
---
Outside diff comments:
In `@landing-page/scripts/auth_api.py`:
- Around line 229-244: Update init_db’s exception handling so caught DDL errors
cannot return as successful initialization after con.rollback(). Serialize
schema setup with an advisory lock or retry the complete transaction, and if
initialization still fails, re-raise the error after closing the cursor and
releasing the connection; preserve cleanup in finally.
In `@landing-page/scripts/tasks_predict.py`:
- Around line 135-143: Update run_staffline_detection and its call site in the
image-processing flow to accept a cancellation callback, passing
job_store.check_cancelled from the task. Invoke the callback inside the
staffline detection loop so cancellation is observed during processing, while
preserving the existing event publishing behavior.
- Around line 135-143: Update the control flow in the prediction loop so the
`continue` associated with `has_text_alignment` only skips text-finding, not
staffline processing. Ensure the `has_class(yolo_txt, STAFFLINE_CLASS_ID)`
branch still invokes `run_staffline_detection` and publishes its events when
`has_text_alignment` is true and `has_annotation` is false, allowing
`staffline_stage.py` results to be persisted in `staffline_detections`.
🪄 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: e3578607-71f9-4fab-9462-be182fa21a85
📒 Files selected for processing (3)
landing-page/scripts/auth_api.pylanding-page/scripts/tasks_predict.pystaff-finding/scripts/script_tests/test_run_page.py
Two CodeRabbit findings on PR 53, both about run_staffline_detection: its docstring promised a status=failed row on error that the code never wrote (implemented it, on a fresh transaction after the rollback so a secondary DB failure cannot cascade), and cooperative cancellation was only checked once per image in tasks_predict.py, not inside this stage's own per-box loop -- exactly the gap CLAUDE.md warns a new long-running stage needs to avoid. Added a check_cancelled call per box, with JobCancelled explicitly re-raised past the broad except below it so a cancelled job actually stops instead of being recorded as one failed staffline attempt while the outer loop keeps going. Also fixes a control-flow bug in tasks_predict.py found in the same review: the has_text_alignment skip's continue sat before the staffline-detection block, so an image with fresh YOLO boxes but an already-existing text alignment never got a staffline_detections row created at all. Staffline detection is now gated only on has_class, ordered before the has_text_alignment check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ons in run_page.py Two CodeRabbit findings on PR 53, both in run_page.py. First: it imported bgr_adapter at module top, so --no-bgr (and even --help) still needed the external inference_simple dependency to resolve before either could run. Moved the import into process_page's use_bgr branch, the one place it is actually needed; the DEFAULT_BGR_* constants used as function/argparse defaults are now plain literals mirroring bgr_adapter's own, since those still need to exist at module load time regardless. Confirmed directly that a bare import run_page no longer touches sys.modules for bgr_adapter or inference_simple at all, and that the real BGR path still imports it correctly when use_bgr=True is actually requested. Second: run_fallback_redetect computed box_index as len(fit_results) before any candidate had been appended, so every candidate probed for one region wrote its diagnostic PNGs to the same box_XXXX_fallback.png pair, each overwriting the last; the acceptance loop then recomputed a different box_index anyway, so the summary row ended up pointing at a filename nothing had actually written. Gave each candidate its own per-region, per-candidate-index path at the moment its diagnostics are written, tracked in a dict keyed by the candidate's own identity, and carried that exact path through to its summary row instead of reconstructing a new one. Verified with a real under-populated-stave scenario producing 3 accepted candidates: no filename collisions, and every summary row's path corresponds to a file actually on disk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g line CodeRabbit, PR 53. validate_and_select_candidates ranked and capped fallback candidates purely by confidence, with no check against the stave's own already-detected lines. A candidate that was really just a noisier re-detection of an existing line could out-score a genuinely missing line and win one of the limited max_new_lines slots, permanently displacing the line the whole probe was meant to recover. group_staves own duplicate reconciliation would eventually collapse such a duplicate too, but only after that displacement had already happened. Added existing_centers to ProbeRegion (populated from identify_probe_regions own centers list, which it already computed), a DUPLICATE_EXISTING_LINE_TOLERANCE_RATIO=0.5 constant, and a _duplicates_existing_line check wired into the same plausibility filter as the existing territory/width/score checks, so a duplicate is rejected before ranking rather than after. New test constructs the exact adversarial case: a high-confidence duplicate 3px from an existing line competing against a lower-confidence but genuinely new line, with max_new_lines=1. Confirmed the genuinely new line wins and cap_exceeded is false, meaning the duplicate was rejected outright rather than merely losing a tiebreak. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeRabbit, PR 53. _pred_page_y/_pred_page_x's legacy fallback path (for records without centerline_page) unconditionally indexed item[bounding_box][uly/ulx]. Interpolated JSOMR records never have a bounding_box (there is no crop of their own to have one), so a stale pre-centerline_page fixture containing an interpolated line crashed with a TypeError on None[uly] instead of just having nothing to contribute to the comparison. Both functions now return an empty array when bounding_box is None and there is no centerline_page to fall back to first, matching how the rest of eval_page.py already treats an empty prediction. The double-page-offset issue for stale fixtures that predate centerline_page entirely is a fixture-regeneration problem, not something this function can paper over, and is left as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r neighbours CodeRabbit, PR 53 (experiments/dp_tracing). The DP cost only rewarded dark pixels, so whenever the search band overlapped a neighbouring staffline, the trace could converge onto that darker neighbour instead of following the YOLO-box hint it started from. Added a distance-from-hint penalty, normalized to [0,1] across the band and computed once (the hint does not move column to column), added into both the first columns dp_prev and every subsequent columns dp_col. Weight is empirical, not guessed: reproduced the drift on a synthetic band with a faint true line and a darker, thicker distractor 12px from the hint, swept weights from 0.3 to 2.0, found 0.6 as the actual tipping point where the distractor stops winning, and set the default to 0.7 for margin. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fline CodeRabbit, PR 53 (experiments/periodicity). periodicity_trace recomputed the full-page Gaussian blur and darkness normalisation on every call, even though both depend only on gray and scale_unit, which are invariant across an entire page -- a page with N stafflines redid this full-page work N times instead of once. Extracted the computation into compute_dark_field(gray, scale_unit, blur_sigma_multiplier), and gave periodicity_trace an optional dark parameter that reuses a caller-supplied field instead of recomputing, falling back to computing it internally when not given so existing callers are unaffected. run_periodicity_page now computes it once before the per-staffline loop and passes it into every periodicity_trace call. Verified output is byte-identical before and after this change on the same input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeRabbit, PR 53. A per-image exception was caught, printed, and skipped with no structural record and no effect on the exit code, so a batch run where every image failed looked identical, from the outside, to one where every image succeeded -- any automation checking only the exit code would see a false success. run_inference now collects failures as it goes and writes failed_images.json (image path + error) alongside the existing all_predictions.json when non-empty, and returns the failure list to main(), which now exits 1 on any partial failure. all_predictions.json itself is unchanged in shape (still a bare list) so existing annotator-side consumers are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… Dockerfile Four small independent fixes bundled together: group_staves.py: interpolation_max_gap docstring claimed it always defaults to cut_threshold * INTERPOLATION_GAP_MULTIPLIER when None, but the adaptive gap-distribution path in interpolate_staves._compute_interpolation_max_gap is what actually resolves it in the normal case, with that multiplier only as its own fallback when no gaps are available. Docstring now describes the real resolution path. .gitignore: !models/**/*.pt only rescues the repo-root models/ directory, not landing-page/scripts/assets/models/medieval/ -- those .pt files are tracked today only because gitignore does not apply retroactively, so a future accidental untrack would silently need -f to re-add them. Added a path-specific negation. Confirmed via git check-ignore that this path specifically needed its own rule, and left staff-finding/models/ untouched since those files are genuinely, intentionally untracked. pyproject.toml: gp_fitter.py needs scikit-learn and implicit_neural_fitter.py needs torch, neither declared anywhere despite both being real experiment runners. Added both as a new experiments extra, kept separate from experiments/ itself (which stays outside py-modules, unshipped to production). landing-page/Dockerfile: switched the staff-finding install from editable to normal, since the whole source tree is already baked into the image via the COPY above regardless -- -e buys nothing at runtime and no live-edit workflow makes sense inside a built container. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a CodeRabbit review pass section to STAFFLINE_INTEGRATION_FOLLOWUPS.md listing every fix made in the preceding commits, plus what was declined (the documented same-stem-filename-collision limitation) and what was agreed with but deliberately deferred rather than rushed. Also adds two new deferred items surfaced by the review itself: component_filter.py's discarded/kept bookkeeping not accounting for companion retention (Minor, needs more careful reading of an 811-line file than a Minor finding warrants in the same pass), and staffline_detections having no retention/archival plan yet (same category of gap as the existing job_uploads/job_sessions TODO, worth planning alongside those rather than in isolation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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)
scripts/run_inference.py (1)
71-82: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle failures in the complete per-image workflow.
The handler at Line 79 only covers
model.predict. An exception fromresults[0],result.plot(), annotation conversion, or per-image JSON output terminates the batch beforefailed_images.jsonis written.cv2.imwritecan also returnFalsewithout raising.Keep all per-image processing inside this handler. Check the return value from
cv2.imwrite. Record the image failure and continue when any per-image operation fails.🤖 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 `@scripts/run_inference.py` around lines 71 - 82, Extend the per-image try/except in the workflow containing model.predict to cover results[0], result.plot(), annotation conversion, JSON output, and image writing. Validate cv2.imwrite’s boolean return and raise or handle a failure when it is false, then record the image and error in failed_images and continue so batch processing reaches failed_images.json.
🧹 Nitpick comments (1)
documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md (1)
193-196: 📐 Maintainability & Code Quality | 🔵 TrivialMake
tests / testa required merge check.The workflow runs on push, but
maindoes not require the status check. A failing test run can still merge. Enable branch protection before relying on this workflow as a release gate.🤖 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 `@documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md` around lines 193 - 196, Update the repository’s branch protection configuration for main so the tests / test status check is required to pass before merging. Enable this setting in the main branch protection rule and preserve the existing tests workflow 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 `@documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md`:
- Around line 73-78: Update the staffline_detections follow-up in
STAFFLINE_INTEGRATION_FOLLOWUPS.md to require bounded retention or archival
before production use, while preserving the comparison window needed by
interpolate_staves. Alternatively, explicitly state that production use is
deferred until such a bound is implemented.
- Around line 60-67: Update the test stubs in test_run_page.py and
test_run_pageOG.py to use a monkeypatch-backed fixture or explicitly restore
each original sys.modules entry after every test. Ensure the fake
inference_simple and bgr_adapter modules are removed or replaced with their
original values during teardown so no test observes leaked module state.
---
Outside diff comments:
In `@scripts/run_inference.py`:
- Around line 71-82: Extend the per-image try/except in the workflow containing
model.predict to cover results[0], result.plot(), annotation conversion, JSON
output, and image writing. Validate cv2.imwrite’s boolean return and raise or
handle a failure when it is false, then record the image and error in
failed_images and continue so batch processing reaches failed_images.json.
---
Nitpick comments:
In `@documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md`:
- Around line 193-196: Update the repository’s branch protection configuration
for main so the tests / test status check is required to pass before merging.
Enable this setting in the main branch protection rule and preserve the existing
tests workflow behavior.
🪄 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: ca615335-2503-4244-9d5b-0b6c3a3653d9
📒 Files selected for processing (15)
.gitignoredocumentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.mdlanding-page/Dockerfilelanding-page/scripts/staffline_stage.pylanding-page/scripts/tasks_predict.pyscripts/run_inference.pystaff-finding/experiments/dp_tracing/dp_tracer.pystaff-finding/experiments/periodicity/periodicity_detector.pystaff-finding/experiments/periodicity/run_periodicity_page.pystaff-finding/pyproject.tomlstaff-finding/scripts/eval_page.pystaff-finding/scripts/fallback_redetect.pystaff-finding/scripts/group_staves.pystaff-finding/scripts/run_page.pystaff-finding/scripts/script_tests/test_fallback_redetect.py
🚧 Files skipped from review as they are similar to previous changes (10)
- landing-page/Dockerfile
- landing-page/scripts/tasks_predict.py
- staff-finding/experiments/dp_tracing/dp_tracer.py
- .gitignore
- staff-finding/scripts/eval_page.py
- staff-finding/experiments/periodicity/run_periodicity_page.py
- staff-finding/scripts/fallback_redetect.py
- staff-finding/scripts/script_tests/test_fallback_redetect.py
- staff-finding/scripts/group_staves.py
- staff-finding/scripts/run_page.py
Comment on lines
+60
to
+67
| **Update**: `test_run_page.py` specifically also stubbed the wrong | ||
| module — `sys.modules["inference_simple"]` never took effect because | ||
| `bgr_adapter.py` raises its own `ModuleNotFoundError` before ever reaching | ||
| that import (an unconditional `os.path.isfile()` check across hardcoded | ||
| developer paths). This only surfaced in real CI, not local runs, since | ||
| the local dev machine happened to have the real dependency at one of | ||
| those paths. Fixed by also stubbing `sys.modules["bgr_adapter"]` itself — | ||
| the no-teardown concern above still applies to both files, unfixed. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore sys.modules after each test.
test_run_page.py and test_run_pageOG.py still replace global module entries without teardown. CI isolation hides the leak, but a normal test suite can observe the fake modules. Move the stubs into a fixture with monkeypatch teardown, or restore each original entry.
🤖 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 `@documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md` around lines 60 -
67, Update the test stubs in test_run_page.py and test_run_pageOG.py to use a
monkeypatch-backed fixture or explicitly restore each original sys.modules entry
after every test. Ensure the fake inference_simple and bgr_adapter modules are
removed or replaced with their original values during teardown so no test
observes leaked module state.
The 2026-07-30 merge (15061d1) kept only this branch's own Deployment (Docker) section in CLAUDE.md and silently dropped both of mains -- Deployment (Kubernetes, CI/CD via GitHub Actions), the real production path (auto-deploy to a k8s cluster on push to main), and Local/manual container runs, which explicitly scopes docker-compose.yml as local-only. This branch's text claimed Compose is now the only deployment path, which was wrong -- the actual k8s/ manifests and CI/CD deploy job were untouched the whole time (confirmed byte-identical to main), only the docs were stale. Restored both of mains sections, folding this branchs genuinely new local-testing content (the staff-finding Docker build-context wiring, buildx/OOM/Tridis/redeploy-together notes) into the restored Local/manual section. Also fixed a smaller staleness bug in the same paragraph (still said pip install -e for staff-finding, changed to non-editable by an earlier commit this session), refreshed three Key files table rows that had fallen behind main (job_store.py/jobs_api.py/tasks_predict.py+tasks_encode.py, missing cancel/retry and tasks_text_batch.py mentions), and added the staffline_detections row to the Database schema table, which a previous summary had incorrectly claimed already existed. Found while evaluating what a rebase of this branch onto main would take -- see documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md for the full evaluation and why a rebase was decided against in favor of a squash-merge at PR #53 landing time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kyrieb-ekat
changed the title
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)
CLAUDE.md (1)
269-269: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language tag to the shell code fence.
Use
```bashfor this block. markdownlint reports MD040 on Line 269.Proposed fix
-``` +```bash🤖 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.md` at line 269, Update the shell code fence at the affected documentation block in CLAUDE.md to use the bash language tag, changing the opening fence to ```bash while preserving the block’s contents.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 `@CLAUDE.md`:
- Around line 310-315: The documentation around the staff_finding build wiring
incorrectly describes the failure as occurring at runtime. Update the relevant
CLAUDE.md guidance to state that omitting staff_finding causes the backend and
worker Compose builds and the GitHub Actions backend build to fail during Docker
image construction at COPY --from=staff_finding, before any image is produced.
- Line 48: Update project and account deletion flows to remove associated
staffline_detections rows before deleting the project, preserving their
historical accumulation during normal operation. In the staffline_adapter.py
latest-row lookup, extend the created_at DESC ordering with a unique descending
id tie-breaker so selection is deterministic.
---
Outside diff comments:
In `@CLAUDE.md`:
- Line 269: Update the shell code fence at the affected documentation block in
CLAUDE.md to use the bash language tag, changing the opening fence to ```bash
while preserving the block’s contents.
🪄 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: 21b72c45-e38f-4739-9f58-1ea38b137bd4
📒 Files selected for processing (2)
CLAUDE.mddocumentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md
| | `activity_log`, `project_logs` | audit trail per project | | ||
| | `annotations` | YOLO detections per image (`yolo_txt`), written by the predict job | | ||
| | `text_alignments` | text-finding output per image, written by the predict job | | ||
| | `staffline_detections` | per-image JSOMR staffline detections (`jsomr_json`), written by the predict job — accumulate-forever, unlike `annotations`'s delete-then-insert; see **Staffline detection** below | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
'staffline_detections|staffline_adapter|jsomr_json|INSERT INTO|DELETE FROM|ORDER BY' \
landing-page/scripts staff-finding --glob '*.py'Repository: DDMAL/mothra
Length of output: 50368
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- schema and indexes ---'
sed -n '145,185p' landing-page/scripts/auth_api.py
sed -n '205,235p' landing-page/scripts/auth_api.py
printf '%s\n' '--- current detection lookup ---'
sed -n '35,78p' landing-page/scripts/tasks_encode.py
printf '%s\n' '--- detection writes ---'
sed -n '180,305p' landing-page/scripts/staffline_stage.py
printf '%s\n' '--- project deletion ---'
sed -n '245,272p' landing-page/scripts/projects_api.py
printf '%s\n' '--- adapter selection behaviour ---'
rg -n -C 4 'def staves_from_jsomr|created_at|status|stave_id|within_stave_index' \
landing-page/scripts/staffline_adapter.py landing-page/scripts/tests/test_staffline_adapter.pyRepository: DDMAL/mothra
Length of output: 20340
Delete staffline detections with their project and make latest-row selection deterministic.
staffline_detections intentionally retains history, but project and account deletion omit this table. Its project_id foreign key can therefore prevent project deletion after detections exist. Add project-scoped cleanup. Also add a unique tie-breaker, such as id DESC, to the created_at DESC lookup; staffline_adapter.py only processes the selected row.
🤖 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.md` at line 48, Update project and account deletion flows to remove
associated staffline_detections rows before deleting the project, preserving
their historical accumulation during normal operation. In the
staffline_adapter.py latest-row lookup, extend the created_at DESC ordering with
a unique descending id tie-breaker so selection is deterministic.
…line_detections cleanup, sys.modules leak, stale fixtures Went through every still-open CodeRabbit thread on PR #53 individually (14 unresolved out of 31 total -- the other 17, including the three latest-round threads on periodicity_detector.py/.gitignore/pyproject.toml, turned out to already be fixed by earlier commits this session and just needed the GitHub thread itself resolved, not a code change). tasks_predict.py: has_annotation is read during the validating stage, then re-fetched later by a separate statement when actually processing the image. If the annotation was deleted in between (e.g. a concurrent duplicate job), fetchone() returned None and the unpack raised TypeError, aborting the whole predict task. Now falls through to a fresh YOLO run instead of crashing when the row has disappeared. staffline_detections: project and account deletion (projects_api.py, account_api.py) never cleaned up this table, so its project_id FK could block both DELETE FROM projects paths once a project had any detections. Added the missing DELETE FROM staffline_detections to both. Also gave tasks_encode.py's latest-row lookup (ORDER BY created_at DESC) an id DESC tie-breaker, since staffline_adapter.py only ever sees the single row that query picks. CLAUDE.md: corrected the documented failure mode for a missing staff_finding build context -- it fails docker compose build / the GitHub Actions build itself at COPY --from=staff_finding, before any image exists, not silently at runtime in a shipped image. heatmap_regression/NOTES.md: this unimplemented experiment's supervision-signal section prescribed flat YOLO box-centre targets (same y repeated across the whole box width) as sufficient ground truth for a model meant to output a curved, sub-pixel centerline -- flat targets reward exactly the wrong shape. Now prefers real per-column centerline annotations where available, and frames flat-target training as coarse first-stage supervision requiring a refinement step, not a first model. test_run_page.py / test_run_pageOG.py: both replace sys.modules entries (torch/ultralytics/inference_simple/bgr_adapter) at import time with no teardown, confirmed to leak into and break later test files when collected in the same pytest session (that's why tests.yml runs them as separate steps). The stubbing itself still has to happen at module level -- it has to exist before this same file's own later `import run_page` -- but it's now done through pytest.MonkeyPatch() instead of raw sys.modules[...] assignment, with a module-scoped autouse fixture calling .undo() once the file's tests finish. Verified the leak is actually gone by running both files ahead of test_fit_centerline.py in one session (previously contaminated, now passes). Regenerated all four tracked implicit_neural* Gent-right fixtures via run_implicit_neural_page.py: they predated the JSOMR writer's centerline_page field, so interpolated (null-bounding_box) records would crash eval_page.py and detected records risked double-applying the page offset. Every record in all four now carries centerline_page; verified clean against eval_page.py's own _pred_page_y/_pred_page_x helpers. Investigated but deliberately did not touch: the three checked-in stave_grouping_report.txt baselines CodeRabbit flagged as fragmented (Gent right, F-Pn-Latin-15181_107r, F-Pn-Latin-15181_221). Re-ran all three through the current pipeline rather than assuming staleness -- two reproduce their checked-in duplicate-fit/unexpected-count numbers almost exactly even past this session's fallback-redetect fix, and the third shows the same symptom under two different candidate inputs. This is a real, currently-unresolved group_staves.py reconciliation gap on dense/tightly-spaced staves, not a stale fixture -- written up in STAFFLINE_INTEGRATION_FOLLOWUPS.md rather than papered over with a low-confidence patch or a fixture swap that would just check in a different flavor of the same fragmentation. Same treatment for the detect_stafflines.py/test_model.sh same-stem-collision finding: still a real, documented, deliberately-accepted limitation (test_model.sh's own help text), not something this pass changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@landing-page/scripts/account_api.py`:
- Around line 111-112: Serialize project deletion in both
landing-page/scripts/account_api.py (lines 111-112) and
landing-page/scripts/projects_api.py (lines 262-265) with staffline writes by
using one transaction/pooled connection and locking the target projects via
SELECT ... FOR UPDATE before deleting child rows and the parent; preserve the
existing deletion behavior while preventing concurrent inserts from racing the
final project delete.
In `@staff-finding/scripts/script_tests/test_run_page.py`:
- Around line 70-76: Refactor both
staff-finding/scripts/script_tests/test_run_page.py (lines 70-76) and
staff-finding/scripts/script_tests/test_run_pageOG.py (lines 42-45) so fake
module setup, sys.path changes, target imports, and tests execute within a
single module-scoped fixture rather than during collection. Ensure fixture
teardown removes the fake torch, ultralytics, inference_simple, and bgr_adapter
entries from sys.modules, including any cached references created by imported
targets, and remove the existing collection-time setup/restore flow.
🪄 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: f25b6aa3-58ba-4b12-88c5-3348ce54f3cf
📒 Files selected for processing (13)
CLAUDE.mddocumentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.mdlanding-page/scripts/account_api.pylanding-page/scripts/projects_api.pylanding-page/scripts/tasks_encode.pylanding-page/scripts/tasks_predict.pystaff-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/experiments/heatmap_regression/NOTES.mdstaff-finding/scripts/script_tests/test_run_page.pystaff-finding/scripts/script_tests/test_run_pageOG.py
🚧 Files skipped from review as they are similar to previous changes (4)
- CLAUDE.md
- landing-page/scripts/tasks_predict.py
- landing-page/scripts/tasks_encode.py
- staff-finding/experiments/heatmap_regression/NOTES.md
Comment on lines
+111
to
112
| cur.execute("DELETE FROM staffline_detections WHERE project_id=%s", (pid, )) | ||
| cur.execute("DELETE FROM projects WHERE user_id=%s", (user["id"], )) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant files:\n'
git ls-files | rg '(^|/)landing-page/scripts/(account_api|projects_api|staffline_stage|auth_api|config|tasks_.*\.py)\.py$|CLAUDE\.md$|.*schema.*|.*database.*|.*config\.yaml$' | head -200
printf '\naccount_api.py outline (if present):\n'
ast-grep outline landing-page/scripts/account_api.py --view expanded || true
printf '\nprojects_api.py outline (if present):\n'
ast-grep outline landing-page/scripts/projects_api.py --view expanded || true
printf '\nstaffline_stage.py outline (if present):\n'
ast-grep outline landing-page/scripts/staffline_stage.py --view expanded || true
printf '\nRelevant slices:\n'
printf '\n-- account_api.py 80-130 --\n'
sed -n '80,130p' landing-page/scripts/account_api.py | cat -n
printf '\n-- projects_api.py 230-285 --\n'
sed -n '230,285p' landing-page/scripts/projects_api.py | cat -n
printf '\n-- staffline_stage.py references to staffline_detections ---\n'
rg -n "staffline_detections|insert into|INSERT INTO|project_id|commit|cursor|execute|get_db_conn|require_project_owner" landing-page/scripts/staffline_stage.py landing-page/scripts/account_api.py landing-page/scripts/projects_api.py
printf '\n-- schema/table definitions mentioning staffline_detections and foreign keys ---\n'
rg -n -i "staffline_detections|projects|foreign key|REFERENCES|REFERENCES projects|FK|CREATE TABLE" -S . -g '!node_modules' -g '!dist' -g '!build' | head -300Repository: DDMAL/mothra
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '-- auth_api.py db_cursor and helpers --\n'
sed -n '1,90p' landing-page/scripts/auth_api.py | cat -n
printf '\n-- staffline_stage.py run_staffline_detection and staffline_detections INSERTs --\n'
sed -n '170,295p' landing-page/scripts/staffline_stage.py | cat -n
printf '\n-- staffline_stage.py cursor/connection references --\n'
rg -n "with db_cursor|get_db_conn|con.commit|cur.execute|staffline_detections|run_staffline_detection|queue_size|submit|tasks_.*staffline" landing-page/scripts/staffline_stage.py landing-page/scripts/tasks_*.py landing-page/scripts/main.py
printf '\n-- read-only source verifier for FK/lock interleaving risk --\n'
python3 - <<'PY'
from pathlib import Path
import re
paths = {
"account_delete": Path("landing-page/scripts/account_api.py"),
"project_delete": Path("landing-page/scripts/projects_api.py"),
"staffline_write": Path("landing-page/scripts/staffline_stage.py"),
"schema": Path("landing-page/scripts/auth_api.py"),
}
for name, path in paths.items():
print(f"\n### {name}: {path} exists={path.exists()}")
if not path.exists():
continue
s = path.read_text()
print(name, "has staffline_detections references:", "staffline_detections" in s)
print(name, "has db_cursor:", "db_cursor" in s)
print(name, "has com" in s.lower())
PYRepository: DDMAL/mothra
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '-- auth_api.py db_cursor and helpers --'
sed -n '1,100p' landing-page/scripts/auth_api.py | cat -n
printf '%s\n' '\n-- staffline_stage.py run_staffline_detection and staffline_detections INSERTs --'
sed -n '170,310p' landing-page/scripts/staffline_stage.py | cat -n
printf '%s\n' '\n-- staffline_stage.py cursor/connection references --'
rg -n "with db_cursor|get_db_conn|con.commit|cur.execute|staffline_detections|run_staffline_detection|queue_size|submit|tasks_.*/staffline" landing-page/scripts/staffline_stage.py landing-page/scripts/tasks_*.py landing-page/scripts/main.py || true
printf '%s\n' '\n-- read-only source verifier for FK/lock interleaving risk --'
python3 - <<'PY'
from pathlib import Path
for path in [
Path("landing-page/scripts/account_api.py"),
Path("landing-page/scripts/projects_api.py"),
Path("landing-page/scripts/staffline_stage.py"),
Path("landing-page/scripts/auth_api.py"),
]:
text = path.read_text()
print(f"\n### {path}: exists={path.exists()}")
for label, value in [
("staffline_detections references", "staffline_detections" in text),
("db_cursor", "db_cursor" in text),
("staffline_detects and commit", "staffline_detections" in text and "con.commit" in text),
]:
print(label, value)
PYRepository: DDMAL/mothra
Length of output: 14663
Serialize the project delete paths with the staffline write path.
staffline_detections.project_id references projects.id, and staffline detection commits after inserting rows. The delete paths select rows, delete children, then delete the parent on separate statements; a worker can insert a new child for the same project between those steps and make the final DELETE FROM projects fail. Hold one transaction for the delete path with SELECT ... FOR UPDATE, or have the delete path use the same pooled connection/transaction as staffline writes.
📍 Affects 2 files
landing-page/scripts/account_api.py#L111-L112(this comment)landing-page/scripts/projects_api.py#L262-L265
🤖 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 `@landing-page/scripts/account_api.py` around lines 111 - 112, Serialize
project deletion in both landing-page/scripts/account_api.py (lines 111-112) and
landing-page/scripts/projects_api.py (lines 262-265) with staffline writes by
using one transaction/pooled connection and locking the target projects via
SELECT ... FOR UPDATE before deleting child rows and the parent; preserve the
existing deletion behavior while preventing concurrent inserts from racing the
final project delete.
Comment on lines
+70
to
+76
| @pytest.fixture(scope="module", autouse=True) | ||
| def _restore_stubbed_modules(): | ||
| """Undo the sys.modules stubs above once this file's tests are done, | ||
| instead of leaking fake torch/ultralytics/inference_simple/bgr_adapter | ||
| into whatever test file pytest collects next.""" | ||
| yield | ||
| _mp.undo() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files | rg '(^|/)test_run_page(\.OG)?\.py$|staff-finding/scripts/script_tests|script_tests'
echo
echo "== outlines =="
ast-grep outline staff-finding/scripts/script_tests/test_run_page.py --view expanded || true
echo
ast-grep outline staff-finding/scripts/script_tests/test_run_pageOG.py --view expanded || true
echo
echo "== relevant contents =="
cat -n staff-finding/scripts/script_tests/test_run_page.py
echo
echo "========================================"
cat -n staff-finding/scripts/script_tests/test_run_pageOG.py
echo
echo "== related patch helper =="
rg -n "_mp|undo|stubbed|sys\.modules|sys\.path|`@pytest.fixture`|from .* import|import " staff-finding/scripts/script_tests -SRepository: DDMAL/mothra
Length of output: 20844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pytest install/config files =="
git ls-files | rg '(^|/)(pytest\.ini|pyproject\.toml|setup\.cfg|tox\.ini|requirements.*\.txt|poetry\.lock|Pipfile)$' | xargs -r sed -n '1,220p'
echo
echo "== pytest availability/pytest_collectreport fixture semantics =="
python3 - <<'PY'
import importlib.util, types
spec = importlib.util.find_spec("pytest")
print("pytest", "installed" if spec else "not installed")
PY
echo
echo "== deterministic Python import/collect ordering probe =="
python3 - <<'PY'
import sys, types, importlib.abc
print("python", sys.version.split()[0])
order = []
current_modules = set(sys.modules)
class FakeLoader(importlib.abc.Loader):
def exec_module(self, mod):
order.append(('exec', mod.__name__))
mod.value = 7
class FakeFinder(importlib.abc.MetaPathFinder):
def find_spec(self, fullname, path=None, target=None):
order.append(('find', fullname))
if fullname in ("fake", "later"):
spec = importlib.util(spec=None, loader=FakeLoader(), origin="fake.py", is_package=False)
spec.loader = FakeLoader()
return spec
if fullname == "test_run_page_type":
src = 'import fake\nsys.modules["test_run_page_type"] = __import__("test_run_page_type")\nsys.path.insert(0, "/tmp/stubs")\n'
spec = importlib.util.spec_from_loader("test_run_page_type", FakeLoader())
spec.loader = FakeLoader()
return spec
return None
sys.meta_path.insert(0, FakeFinder())
sys.path.insert(0, "/tmp/stubs")
stubs = (sys.modules.get("fake"), sys.modules.get("later"), sys.modules.get("test_run_page_type"))
__import__("test_run_page_type")
later = __import__("later")
del sys.meta_path[0]
del sys.path[0]
print("order:", order)
print("fake_late_value_during_first_import_faked:", later.value, "type_of_fake_during_first_import_faked:", type(fake))
print("later_value_after_collection_patch_removal:", later.value, "later_value_after_fixture_patch_removal:", later.value, "type_of_fake_after_fixture_patch_removal:", type(fake))
print("sys.modules_later_after_fixture_patch_removal_present:", "later" in sys.modules)
PYRepository: DDMAL/mothra
Length of output: 4006
🏁 Script executed:
#!/bin/bash
set -auo pipefail
echo "== relevant file sizes and lines =="
wc -l staff-finding/scripts/script_tests/test_run_page.py staff-finding/scripts/script_tests/test_run_pageOG.py
sed -n '66,67p' staff-finding/scripts/run_page.py
echo
sed -n '1,80p' staff-finding/scripts/yolo_io.py
echo "== targeted imports in run_page.py =="
rg -n '^\s*(import|from)\s+(inference_simple|bgr_adapter|torch|ultralytics)\b|^importlib|^sys\.' staff-finding/scripts/run_page.py -S || true
echo
echo "== deterministic Python import/collected-file ordering probe =="
python3 - <<'PY'
import sys, types, importlib.util, importlib.abc
order = []
sys.meta_path.insert(0, type("Finder", (importlib.abc.MetaPathFinder,), {
"find_spec": lambda self, name, path=None, target=None: (
order.append(("find", name)) or importlib.util.spec_from_loader(name, type("Loader", (importlib.abc.Loader,), {
"exec_module": lambda self, m: order.append(("exec", m.__name__))
})()) if name in ("coll", "coll2") else None
)
})())
# Pytest collection has at least one collection step before module-scoped fixture setup.
# This probe models the same Python import behavior that applies when a test file is collected.
__import__("coll")
order.append(("fixture_after_collect", list(order)))
__import__("coll2")
order.append(("collection_complete", False))
print("order:", order)
PYRepository: DDMAL/mothra
Length of output: 3441
Move fake imports out of module execution.
test_run_page.py and test_run_pageOG.py set sys.modules stubs and modify sys.path during file collection, then undo them only in a module-scoped autouse fixture. A later pytest collection pass that imports a target module before that fixture starts can receive fake torch, ultralytics, inference_simple, or bgr_adapter references. Put the stub setup, target import, and tests in one module-scoped fixture, and ensure the fake entries are removed from sys.modules if imported targets can cache them.
📍 Affects 2 files
staff-finding/scripts/script_tests/test_run_page.py#L70-L76(this comment)staff-finding/scripts/script_tests/test_run_pageOG.py#L42-L45
🤖 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_page.py` around lines 70 - 76,
Refactor both staff-finding/scripts/script_tests/test_run_page.py (lines 70-76)
and staff-finding/scripts/script_tests/test_run_pageOG.py (lines 42-45) so fake
module setup, sys.path changes, target imports, and tests execute within a
single module-scoped fixture rather than during collection. Ensure fixture
teardown removes the fake torch, ultralytics, inference_simple, and bgr_adapter
entries from sys.modules, including any cached references created by imported
targets, and remove the existing collection-time setup/restore flow.
Staffline detection has been running as part of every predict job for a while, writing to staffline_detections, but had zero frontend surface -- CLAUDE.md's own deferred-items list already flagged this. This is that UI pass: a Stafflines tab alongside Annotations and Text, following their established patterns rather than inventing new ones.
Backend: no read endpoint for staffline_detections existed at all (confirmed by grepping every router; the only existing reads are server-side-only, inside tasks_encode.py's MEI logic). Added a _map_staffline_row + a stafflines array wired into the same project payload that already carries annotations/textAlignments (projects_api.py), and a GET /projects/{id}/stafflines/{detection_id} detail route mirroring the existing annotation/text-alignment GETs exact JOIN-based ownership check (inference_api.py). jsomr_json is native JSONB, unlike yolo_txt/alignment_json -- returned as a plain array, no json.loads needed.
Frontend: staffline_detections accumulates forever (multiple rows per image over repeated predict runs), exactly like text_alignments and unlike annotations delete-then-insert -- so StafflinesTab.tsx mirrors TextAlignmentsTab.tsx (rerun-disambiguation labeling, no selection/delete UI), not AnnotationsTab.tsx. StafflineViewerModal.tsx is the first curve-drawing overlay in the frontend (every existing one is fillRect/strokeRect only) -- draws each lines centerline_page polyline color-coded by stave_id, dashed for interpolated lines, and overridden to a warning color for any stave group_staves flagged with a rhythm_status anomaly, plus a light bounding_box outline for context. Wired into ProjectDetail.tsx's existing tab-union/tabs-array/render-block pattern (same stepsUnlocked >= 1 gate as annotations/text), and into AppRouter.tsx's two post-predict-job project-refresh spots so the tab updates live without a full reload.
Verified: tsc -b clean (0 errors). npm run lint shows only pre-existing issues -- confirmed by direct comparison, StafflineViewerModal.tsx's one flagged line is byte-for-byte the same react-hooks/set-state-in-effect pattern already present at the identical spot in both AnnotationViewerModal.tsx and TextAlignmentViewerModal.tsx, the two files this mirrors. Backend logic (positional/keyword argument wiring through the now-17-parameter _build_project_dict, called from both list_projects and _project_row_to_dict) manually re-verified line by line. Not verified: an actual live-server/browser pass -- this session's dev environment has no submodules initialized, no venvs, no node_modules-independent DB, per the earlier gap-check; needs a real DATABASE_URL to test end-to-end.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@landing-page/src/components/project/StafflineViewerModal.tsx`:
- Around line 138-142: Update the anomalousStaveIds count span in
StafflineViewerModal to remove the inline color style and use the static
Tailwind text color utility matching ANOMALY_COLOR, while preserving its
existing typography and conditional rendering.
- Around line 127-130: Replace the custom dialog shell in StafflineViewerModal
with the shared Modal component from Modal.tsx, removing the manually rendered
overlay and positioning wrapper. Preserve the existing staffline viewer body by
passing it as the modal content, and rely on Modal’s established fixed inset-0
z-50 bg-black/60 overlay and close behavior.
- Around line 91-114: Update the useEffect handling staffline image loading to
track the created object URL, revoke it during cleanup, and ignore Promise
completions after unmount. Ensure cleanup runs both when the modal closes and
before effect reruns, while preserving the existing ready and error state
behavior.
In `@landing-page/src/types.ts`:
- Around line 46-52: Update the StafflineSet interface so imageSrc, staveCount,
and modeLinesPerStave accept null values in addition to their existing types,
matching the API and database responses while preserving their optional status.
🪄 Autofix
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: eb151bcc-0e75-4d58-abc6-e512a61d437f
📒 Files selected for processing (7)
landing-page/scripts/inference_api.pylanding-page/scripts/projects_api.pylanding-page/src/components/AppRouter.tsxlanding-page/src/components/project/ProjectDetail.tsxlanding-page/src/components/project/StafflineViewerModal.tsxlanding-page/src/components/project/StafflinesTab.tsxlanding-page/src/types.ts
…ht-inference fixture
- Revoke the staffline viewer blob URL on effect cleanup/unmount and guard state updates with a disposed flag, fixing a memory leak and stale-closure bug from the effects empty dependency array - Replace the anomaly-count inline color style with a static Tailwind text-[#FF3B30] class - Widen StafflineSet.imageSrc/staveCount/modeLinesPerStave to allow null, matching what projects_api.py actually returns - Add a comment explaining why the modal shell intentionally matches AnnotationViewerModal custom overlay pattern instead of Modal.tsx, which does not support this viewport-stretched layout Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 5, 2026
Merged
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.
Wires the staff-finding module's staffline detection (component filtering
-> Huber centerline fit -> stave grouping) into landing-page's predict/
encode Celery pipeline, per the pipeline design in
github.com//discussions/95. Addresses #138
(pyproject.toml) so landing-page can import it directly.
boxes in tasks_predict.py's per-image loop; results land in a new
staffline_detections table (accumulate-forever, JSOMR-shaped).
staffline_detections (richest) -> YOLO-geometry heuristic -> glyph
clustering (unchanged) -- any project without the new data falls
through to existing behavior untouched.
landing-page/'s build context, actually ships in the backend/worker
images (previously would have silently failed at runtime in production).
a missing failed-row write, a skip-ordering bug, a lazy-import fix for
the unvendored BGR dependency, a fallback-redetect duplicate-line bug,
a crash on legacy JSOMR records, an exit-code bug in the batch inference
script, plus smaller dependency/gitignore/Dockerfile fixes -- see
documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md for the full
list of what was fixed, declined, and deferred.
error in auth_api.py's init_db() from a botched merge, and a scoping
bug in tasks_predict.py's has_annotation/has_text_alignment skip logic.
merge had silently dropped in favor of this branch's local-only Docker
Compose section.
Deferred, tracked in documentation_allons-y/STAFFLINE_INTEGRATION_FOLLOWUPS.md:
ink-separation (BGR) packaging, interpolate_staves corpus validation,
fallback_redetect wiring into the live pipeline, frontend QA UI.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests