Repository navigation
test(cladistics): cover the zero-read half of the vacated-clade tip rule - #246
Merged
Merged
Conversation
…lve conflicts, fix review findings Bring PR #224's resolved #221 changes (BigInt validated run statistics, cladistics/CladeCumulus repairs, CI failure digest) onto current main, handling the four overlapping files: - validated_statistics.jl: keep SQL projection + ordering and BigInt accumulation; adopt #221's pre-allocation MAX_CELLS cell check moved inside the accumulation so an oversized result is refused before it is allocated. - clade_cumulus.jl route: keep main's AbstractString boundary (accepts SubString request fields) and duckdb_tables() view filter; the PR's stricter ::String signature would MethodError on SubString study/run. - clade_cumulus.jl analysis: keep main's correct QueryCandidate field names (distal_len/pendant_len/like_weight); fix the weight-sum check to be asymmetric per Codex P2 — tolerate a lower shortfall down to 1 - PLACEMENT_WEIGHT_SUM_TOL but reject any overweight beyond a tight PLACEMENT_WEIGHT_OVER_TOL bound. - ci.yml: publish a digest of every failing assertion (testset, expression, evaluated values) before the log tail so the Checks API shows the full failure list. - phylogeny.jl: import ..Tools so PH.Tools._tools resolves. - trees.jl: _tree_format/_valid_tree_file accept AbstractString for SubString safety. - server.jl: include cladistics routes once. - tests: zero count in a sample with reads is a defined 0 proportion; fractional JSON counts refused; helper view unregistered after use. Files intentionally not taken from the PR head: src/core/doi.jl (PR drops the `module DOI` wrapper, which is invalid top-level Julia) and src/server/routes/clade_cumulus.jl (main's version is the stricter fix). Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
#232 was cut from `8df1041` and never rebased, so it conflicts with `main` on three files. Merging it here shows what it still adds: almost nothing. Its own review-finding fixes went to `main` through #236 -- "Address PR #232 app comments: asymmetric placement tolerance + import cleanup" -- and #239 carried the corresponding Cladistics work: | #232 change | Where it lives on `main` | |---|---| | `PLACEMENT_WEIGHT_OVER_TOL` + the lower-only shortfall tolerance | `src/analysis/clade_cumulus.jl:55`, applied at `:1052` | | validated-statistics failure digest in `.github/workflows/ci.yml` | `:429` (same 3 000-byte chunks, same 8-chunk cap) | | `_tree_format` / `_valid_tree_file` on `AbstractString` | `src/server/routes/trees.jl:20,22` | | single `include` of `routes/cladistics.jl` | `src/server/server.jl:131` | | `_validated_duckdb_counts` without SQL arithmetic | superseded: `main` streams `DuckDB.StreamResult` partitions and bounds the matrix as features arrive, where #232 sorted the whole table with `ORDER BY feature LIMIT MAX_CELLS + 1` | | `unregister_data_frame` after the replacement scan | `test/unit/test_clade_cumulus.jl:575` | | fractional-JSON 422 case | `test/unit/test_validated_statistics.jl` (already present -- the merge would have duplicated it) | | zero-count proportions, helper-view cleanup | `test/unit/test_exact_summaries.jl`, `test_clade_cumulus.jl` | `import ..Tools` in `src/pipeline/phylogeny.jl` is the one change `main` does not carry, and it is not needed: nothing in `src/` reaches `Tools` through `Phylogeny`, and the FastTree test reaches the registry as `MetaManifold.Tools._tools` (with a comment saying exactly that). Taking the import plus #232's `PH.Tools` test call would have meant adopting both halves for no behaviour change, so `main`'s shape is kept. What survives is the explanatory comment above `unregister_data_frame`: the registration is a connection-local replacement scan, so leaving it in the file makes the view unreadable for every later connection. The branch tree is therefore `main` plus those three comment lines. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
`_reaggregate_after_move` treats a childless clade as a tip when it holds reads *or* when its rank is a sequence terminal. The existing emptied-wrapper test pins the first half (a clade vacated by a move is not a tip). The second half -- a terminal holding zero reads in this table is still a tip -- had no test at all, so nothing would have caught it being dropped. The new case moves a GenusB terminal into a clade holding a read-bearing GenusA tip, a zero-read GenusA tip and an unassigned zero-read tip, and runs both terminal ranks (`"tip"` and `"ASV"`). Two monophyly assertions are the regression guard, and they fail in the right direction if the rule is weakened: with the rule intact GenusA has two tips, so neither reads `true`; drop the rank half and the zero-read tip leaves the global tip count, GenusA looks like a singleton, the read-bearing tip flips to `true` and the zero-read tip gets no verdict (`nothing`) instead of `false`. Taxon purity is asserted too, but as a plain invariant rather than the guard: it is 0.5 either way, because the tie-break between two equally sized genera lands on the same number by a different route. Readings for both the intact and the weakened rule were derived by porting `_reaggregate_after_move` and running the fixture, not by guessing, so the expected values are the algorithm's. Also documents why the `.jplace` test unregisters its `src_df` registration: it is a connection-local replacement scan, and left in the file it makes the view unreadable for every later connection. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 27 minutes. View limit details
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 |
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
Fork governance
🟣 critical · commit
Hypatia scan: 133 finding(s)
panic-attack assail: 17 finding(s)
pons asinorum: 1 finding(s)
rhodibot (RSR compliance): not_run — enrolled, but no report-back was received
panicbot (panic recovery, error handling): not_run — enrolled, but no report-back was received
echidnabot (formal verification: proofs/): not_run — enrolled, but no report-back was received
seambot (integration health: API routes): not_run — enrolled, but no report-back was received
finishingbot (release readiness): not_run — enrolled, but no report-back was received
accessibilitybot (WCAG on the React UI): not_run — enrolled, but no report-back was received
glambot (presentation quality): not_run — enrolled, but no report-back was received
cipherbot: not_applicable — no cryptography is implemented in this repository
sustainabot: not_applicable — fleet adapter slot reserved; no adapter exists yet (gitbot-fleet README)
Scope: static findings reported by each row's tool at the named commit. Purple, red, yellow and blue grade the worst finding present; orange is evidence missing, malformed or a scanner that did not run; grey is an explicit scope exclusion with its reason. No row is a security certification. report.json, badge.json and the raw findings are in the verdict-grid artifact on the linked run. |
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last uncovered piece of the CladeCumulus move logic, plus one comment that was carried across from #232.
This is what remains after #230, #232, #235 and #239 were dispositioned — all four are now closed, with every functional change they made already on
main.What this adds
A test for the second half of the tip rule.
_reaggregate_after_movetreats a childless clade as a tip when it holds reads or when its rank is a sequence terminal:The emptied-wrapper test on
mainpins the first half — a clade vacated by a move stops being a tip. The second half had no test, so nothing would have caught it being dropped. The new case moves aGenusBterminal into a clade holding a read-bearingGenusAtip, a zero-readGenusAtip and an unassigned zero-read tip, and runs both"tip"and"ASV"ranks.The two monophyly assertions are the regression guard, and they fail in the right direction. With the rule intact
GenusAhas two tips, so neither readstrue. Drop the rank half and the zero-read tip leaves the global per-taxon tip count,GenusAlooks like a singleton, the read-bearing tip flips totrue, and the zero-read tip gets no verdict at all (nothing) instead offalse.Taxon purity is asserted as well, but deliberately not as the guard: it reads
0.5either way, because the tie-break between two equally sized genera lands on the same number by a different route. That is noted in the test so a future reader does not treat it as the thing that catches the regression.Every expected value was derived by porting
_reaggregate_after_moveto a scratch script and running the fixture under both the intact and the weakened rule — not by reasoning about it in my head. That mattered: my first draft of the comment claimed purity would move to2/3, which the port showed was wrong.One comment, from #232. Above
DuckDB.unregister_data_framein the.jplacetest: the registration is a connection-local replacement scan, so leaving it in the file makes the view unreadable for every later connection. This was the only line of #232 that had no equivalent onmain.Validation
julang-s3, unreachable here, source-only on the GitHub mirror), so CI is the real check — theJulia testsjob exercises exactly this file. I will follow it and fix anything it turns up.Correction on the record
On #235 I closed with a claim that the "zero-read terminal" testset from a CodeRabbit autofix commit "could not have passed". That was wrong, and I have posted a correction there. It would have passed; the reason I reached a different outcome (taking
main's side in the merge) was thatmainalready had the equivalent rule with the emptied-wrapper assertion — not that the test was broken.