Repository navigation
test(cladistics): correct the jplace weighted UniFrac expectation (follow-up to #239) - #241
Closed
hyperpolymath wants to merge 15 commits into
Closed
hyperpolymath wants to merge 15 commits into
hyperpolymath wants to merge 15 commits into
Conversation
… and repair the CladeCumulus readers `main` still cannot load the package, and the CladeCumulus suite that never ran before now reports twelve defects. All of them are narrowed to: Package loading - src/core/doi.jl: #210 removed `module DOI` but left `end # module DOI`, a lowering error at include time ("end" at top level) — every test file fails before it starts. The opener is restored. - src/server/server.jl: routes/cladistics.jl was included twice, so every /cladistics route is registered twice. One include remains. CladeCumulus readers (test_clade_cumulus.jl, 3 failed / 14 errored) - routes/clade_cumulus.jl opened results.duckdb with `DuckDB.DB` and `DBInterface.connect` without importing either module, so the sources route answered 500 and every HTTP assertion downstream of it failed. Both are imported now, as the other route files already do. - `_graft_jplace_queries` and the diversity block in the route still used the pre-#207 field names (`weight`, `distal_length`, `pendant_length`) on `QueryCandidate` (whose fields are `like_weight`, `distal_len`, `pendant_len`), and `_graft_jplace_queries` used `.edge_num` on `PhyloNode` (whose field is `edge_number`). Five FieldErrors, now aligned with the structs. - parse_newick lost NHX support: the label reader consumed `[&&NHX:B=92]` before the call that records it, so `) [&&NHX:B=92] :0.1` parsed with support `nothing`. The reader now takes the capture refs. - parse_jplace rejected a single-candidate placement whose ratio is below 1.0, which is how placement tools report one site: the remainder is mass they did not place. A single candidate is now decisive when its ratio is at least the new `PLACEMENT_SINGLE_MIN_LWR = 0.5`; a multi-candidate placement must still partition the mass to within `PLACEMENT_WEIGHT_SUM_TOL`. A single candidate below half stays `undecided` with the same reason string. - test_clade_cumulus.jl reached the tool registry as `PH.Tools`, but `Phylogeny` imports `tool_bin` selectively and does not bind `Tools`; `MetaManifold.Tools` is the module that owns the registry. Guards against both merge-harm classes - scripts/check-julia-syntax.jl now rejects a bare `end` at the top level. Such a file parses cleanly and only fails at lowering, which is exactly how a missing `module` opener — the failure that stopped the whole suite — passed the parse gate. - test_routes.jl asserts that no method+path pair is registered twice and that both cladistics route files are present, which no request test could reveal (a duplicate registration silently shadows the first). Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
`main` landed #222 while #223 was open, and the two repaired an overlapping set of CladeCumulus defects by different routes. This merge keeps every fix exactly once and settles the one disagreement between them. Textual conflicts (both sides fixed the same defect): * `read_label_and_quoted` NHX capture — kept main's keyword signature, which the call sites already use, with #223's clearer comment. * `PH.Tools` -> `MetaManifold.Tools` in the FastTree testset — identical on both sides; kept #223's note explaining why. Semantic conflict (git merged both halves into an incoherent rule): * #222 widened `PLACEMENT_WEIGHT_SUM_TOL` from 1e-5 to 0.05 so a lone 0.98 placement would be accepted; #223 kept 1e-5 and added an explicit single-candidate rule. Merging both left the 0.05 slack applied only to multi-candidate partitions, which is where it is least wanted. Resolved to #223's model: a multi-candidate record partitions the mass and must still sum to one within 1e-5, while a single-candidate record is judged on its own share against `PLACEMENT_SINGLE_MIN_LWR`. A truncated 0.96 partition is caught again, and a lone 0.98 still passes. Codex review finding (P1, `routes/clade_cumulus.jl`) — preserve unplaced LWR mass in diversity weights: * Accepting a single candidate down to LWR 0.5 made the diversity block's `cand.like_weight / tw` always 1.0, pushing mass the tool had explicitly left unplaced onto the reported edge and inflating that tip in weighted UniFrac and Faith's PD. Tips are now weighted by the raw like-weight ratio, and the shortfall is reported as a new `unplaced_reads` field per sample rather than renormalised away. * `_graft_jplace_queries` is deliberately unchanged: the clade tree is a count view of the observed table and still conserves every read. The difference between the two consumers is documented at the constants. Also from #223, unchanged: the `module DOI` opener, the duplicate `routes/cladistics.jl` include, the `edge_number`/`like_weight` field names, the stray-top-level-`end` gate in `scripts/check-julia-syntax.jl`, and the duplicate-route-registration test. Tests: the two-rule acceptance model is pinned at the parser, and a new route case drives a `.jplace` study tree through `/clade-cumulus` and asserts both the reported unplaced reads and that the clade tree still conserves all 110 reads.
CI on `main` (8df1041) shows `POST /clade-cumulus` answering 500 for every `study_tree` request: MethodError: no method matching _valid_tree_file(::SubString{String}) `JSON3` hands request fields back as `SubString`s and `strip` keeps them that way, but `_valid_tree_file` and `_tree_format` in `routes/trees.jl` are `String`-typed. The handler has no catch around `_resolve_tree_inputs_from_request`, so the MethodError surfaced as a 500 and took seven assertions down with it: `r_comb.status == 200` failed, and every `comb.*` access after it raised `KeyError` until the testset aborted. #222 fixed this class at `_load_run_table_df` -- "JSON3 hands request fields back as SubStrings; the DuckDB helpers are String-typed, so normalise once at this boundary" -- but `_resolve_tree_inputs_from_request` two functions below kept the unnormalised values. `cladistics.jl` already wraps its own call as `_valid_tree_file(String(published))`, so the convention is to normalise at the call site; this follows it. The existing `r_comb` assertion is the regression test: it sends `"treeFile" => "amplicon.nwk"` over JSON, which is exactly the SubString path that broke. With the `u.edge_num` -> `u.edge_number` repair already carried in from #223, this accounts for 8 of the 9 errors that run reported.
… placed mass Codex raised two findings on the diversity block, and they are the same defect seen from two sides. Retaining the raw like-weight ratio stopped a lone 0.6 placement from being inflated to 1.0 in `mat`, but `weighted_unifrac` then divided by `sum(values(ab))` -- the placed-only mass -- which restored exactly the weight that had just been withheld. The same denominator also erased ASVs the tree does not cover: a sample could be 90% unmapped and still be compared as though every read had been placed. `_sample_total` now lets a caller declare the real library size, and `weighted_unifrac`, `branch_unifrac_contributions` and `unifrac_matrix` take it (`total_a`/`total_b`/`totals`). All default to `nothing` and the mass on the tree, so every existing call is unchanged. A declared total below the on-tree mass is rejected. The route computes one denominator per sample -- the sum of the table column -- and reports `library size - on-tree mass` as `unplaced_reads`, which now covers the like-weight shortfall and uncovered ASVs together. Faith's PD and unweighted UniFrac read presence only and are untouched. Two unrelated defects the suite could not previously reach: `_reaggregate_after_move` built `subtree_pd` by summing each child's branch length plus its subtree, which drops the branch subtending the node itself -- the opposite of `clade_faith_pd`, which the builder uses. Every proposed clade therefore came back short by its own branch and was compared against a current tree measured the other way: moving ASV_4 into a clade of PD 0.60 reported 0.55. It now mirrors `_pd_accum`, and the test pins the exact values plus total-PD conservation. `test_clade_cumulus.jl` bound `const SV`; the thirteen later unit files that bind `SV = MetaManifold.Server` plainly then threw `invalid assignment to constant Main.SV`. The throw escaped the include loop in `runtests.jl`, so roughly thirty unit files and the occupancy probe never ran at all. The binding is now plain, and each file is included inside its own testset so a top-level throw is attributed to its file and does not take the rest of the suite down with it.
With the include loop no longer aborting, `test_differential.jl` and the server smoke tests ran for the first time and found three defects. `_wald` called `erfc`, which exists in no module this project loads -- `SpecialFunctions` is deliberately not a dependency, and the header of `compositional.jl` says so. Every multinomial and Dirichlet-multinomial fit therefore died with `UndefVarError: erfc` the moment it tried to report a p-value, which is to say the models could not be used at all. `compositional_erfc` now sits beside `compositional_loggamma` and `compositional_digamma` and is built the same way, from the regularised incomplete gamma function at a = 1/2: the power series for P below x^2 = 1.5 and the Legendre continued fraction for Q above it, needing only `exp` and `log`. Measured against the C library's erfc the relative error stays under 1e-14 wherever erfc(x) >= 1e-50, i.e. out to a two-sided deviate of |z| = 15, which matches the accuracy the other two already promise. The unit tests pin both sides of the crossover, the reflection, and the two-sided p-value that is its only caller. `min_prevalence` is the second positional argument of `DifferentialConfig`, not a keyword, so the "every taxon filtered out" case constructed a `MethodError` instead of reaching the engine. The same call also passed four samples that were all in group A, which the "each group needs at least one sample" guard would have answered long before the prevalence filter. Both are fixed, so the case now asserts what it says it does.
Main landed #226 while this branch was in review, and the two found three of the same defects independently. Main's resolution is taken for all three, since it is already on main: - `erfc`: main's private `_erfc` replaces the `compositional_erfc` added here. Same algorithm (incomplete gamma at a = 1/2, series below x^2 = 1.5 and a Lentz continued fraction above), so only the name and the tests differ; `compositional.jl` and `test_differential.jl` are taken wholesale from main. - `subtree_pd` in `_reaggregate_after_move`: identical fix, main's `own_pd` naming and one-line post-loop form kept. Git had merged the two variants additively, which would have counted every active internal branch twice; that is resolved to a single addition. - The `Main.SV` collision that was aborting the include loop: main renames this file's binding to `CLADE_SV` instead of dropping `const`. Main's rename is kept. - `min_prevalence` in the multinomial engine test: main's eight-sample table is better than the four-sample one here, and is kept. Kept from this branch, because main does not have them: - The raw like-weight ratio in the diversity block. Main's change to that line only renames `c.weight` to `c.like_weight` and keeps `cand.like_weight / tw`, which is the renormalisation Codex objected to on #223: it hands a lone 0.6 placement a weight of 1.0. The divisor is removed, and the mass the tool never placed is reported as `unplaced_reads`. - `_sample_total` and the `total_a`/`total_b`/`totals` arguments on `weighted_unifrac`, `branch_unifrac_contributions` and `unifrac_matrix`, with the route passing each sample's library size. Without them the metric divides by the placed-only mass and undoes the line above. - Per-file testsets in `runtests.jl`. Main fixed the `SV` collision but not the reason one bad binding could silently skip thirty-five unit files and the occupancy probe. - `_cc_find`, and the drag-and-drop assertions that pin the exact post-move PD values and total-PD conservation rather than comparing a positional index against an inequality.
`main` has moved since #230 opened (#225, #229, #231, #233), so the merge conflicted on seven files. Each conflict is resolved in favour of whichever side is now correct, and the leftovers of #230 are dropped where `main` already carries the same fix by a newer route. * `src/server/routes/clade_cumulus.jl` -- keep `main`'s `with_results_db` helper, its `_clade_cumulus_post_impl` split and its blanket 500 handler (#229) instead of #230's hand-rolled `DuckDB.DB`/`DBInterface` open and the removal of the catch. #230's `treeFile` normalisation comment is kept, since #229 landed the code it describes. * `src/analysis/clade_cumulus.jl` -- keep #230's two-rule jplace rationale and `main`'s `any_active = u.direct_count > 0` rewrite; both are additive. * `test/unit/test_clade_cumulus.jl` -- keep #230's acceptance cases and `main`'s generic `subtree_pd` expectation for the moved clade, which does not hard-code #230's branch lengths. Review findings: * Codex P2 (fully unmapped ASVs were not counted as unplaced) -- fixed by #230's own rewrite: `unplaced_reads` is now the difference between the sample's library size over the whole table and the mass the tree carries, so an ASV the tree does not cover is counted in full. No further change. * Codex P1 (unplaced mass was renormalised away by UniFrac) -- fixed by #230's `totals`/`total_a`/`total_b` denominators, which are preserved through the merge. * CodeRabbit (unplaced-reads description reversed the calculation) -- the unplaced reads are *excluded from the tree-tip abundances* but *included in the library-size denominator*; the comment in `CladeCumulusPanel.tsx` and the `unplaced_reads` doc in `api/types.ts` now say exactly that. * CodeRabbit (a move that empties a clade kept its stale activity) -- still live after the merge, in `_reaggregate_after_move`'s leaf branch: the wrapper around a moved-only child keeps the `cumulative_count` of the subtree that just left, so it stayed "active" with a nonzero `subtree_pd` and frequency. The branch now recomputes the count from `direct_count`, as the internal-node branch already did. A regression test drives `validate_drag_drop` over a tree whose `wrapper` has exactly one child and asserts the emptied clade reads zero count, zero frequency and zero PD. * CodeRabbit (route assertions used `occursin`) -- `test_routes.jl` now asserts whole method+path pairs for the four Cladistics/CladeCumulus routes, so a renamed or longer path can no longer satisfy them. Also: the single-candidate rejection reason is now `single_candidate_below_min_lwr (<lwr>)` rather than `weight_sum_out_of_tolerance`, whose sum is in tolerance by construction; the two tests that pinned the old wording are updated. Frontend: `tsc --noEmit -p .` clean. Julia is not installable here, so the Julia side is checked statically: tree-sitter parses every touched file with no ERROR/MISSING node, and every new field access resolves against the current struct definitions. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
A clade emptied by a drag-and-drop moves leaves with a zero count, zero frequency and zero `subtree_pd` after the previous commit, but it still carried the `residual_count` derived from the subtree that had just left -- so the proposal showed an empty clade inside a positive uncertainty cloud, and its parent added that stale residual a second time on top of the residual the moved subtree reports at its new location. `residual_count` measures uncertainty in the reads a clade holds. A clade left holding none has none to report, so the leaf branch derives it from the same `direct_count` as the count and PD. A clade that still holds reads keeps the residual measured for them. The regression test now pins both sides: the emptied wrapper reports `residual_count == 0`, and the moved clade's residual is unchanged. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
The synchronize event for 58848d9 reached CodeQL but not CI, DOI or Governance, so the Julia suite has not run against the residual fix yet. An empty commit re-fires all of them. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
`main` landed its own answer to most of #230 while this branch was open: #236 adopted the asymmetric placement tolerance (shortfall tolerated to 0.05, excess rejected at 1e-9) and #237 added the unplaced-reads noise floor. So this merge gives up re-litigating the tolerance and keeps `main`'s model verbatim: `PLACEMENT_WEIGHT_SUM_TOL = 0.05`, `PLACEMENT_WEIGHT_OVER_TOL = 1e-9`, the asymmetric parse branch, and the partition expectations that follow from it (0.96 is a truncated candidate list and is `:valid`; 1.04 is overweight evidence and stays `:undecided`). What is kept from this branch is what `main` does not have: * the library-size denominators (`_sample_total`, `totals` / `total_a` / `total_b`) that stop weighted UniFrac renormalising unplaced mass back onto the tips that were placed -- the Codex P1 finding; * the route's whole-table `unplaced_reads`, which counts an ASV the tree does not cover at all -- the Codex P2 finding -- instead of summing per-row placement shortfalls. It subsumes #237's noise floor: subtracting the placed mass from the library size reports a 0.99 placement's 1% exactly, with no threshold to tune; * the emptied-clade fix (`own_count`, `own_residual`) and its regression test; * the exact method+path route assertions; * the corrected unplaced-reads wording in the panel and the API types. The single-candidate reason stays `single_candidate_below_min_lwr (<lwr>)`: under `main`'s asymmetric rule a lone candidate's sum is in tolerance by construction, so the old wording named a weight-sum error that had not happened. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…on, and explorer behavior
…end parsers Carries #230 (via #235, merged onto current main) forward on this branch and answers the review findings still open on #235: - CodeRabbit: a clade emptied by a drag-and-drop move was still counted as a taxon tip in _reaggregate_after_move (inflating purity/monophyly denominators). count_leaf_taxa! and the leaf branch now share one tip eligibility rule: childless nodes count only with direct reads or a terminal tip/ASV rank. Regression test drives the vacated-wrapper scenario with two domains so root purity (0.5 vs 0.6) discriminates. - Codex P2 on #235 (residual on vacated clades) was already fixed by 58848d9 on that branch; verified present after the merge. Security/quality (panic-attack InputBoundary, advisory): - SSE handlers in api/events.ts no longer throw on a malformed frame. - parseJplace reports invalid JSON as a jplace error instead of a raw SyntaxError; the Newick quoted-label reader no longer builds its string by concatenation in a loop (pons string-concat-in-loop). Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
The pinned 0.15636 was written on #235, whose Julia runs after its final merge were all cancelled or skipped, so it was never exercised. CI on #239 (5715 pass, this one failure) evaluates 0.137818..., which is the branch-by-branch value: (0.02x24 + 0.05x24 + 0.02x10 + 0.15x10 + 0.30x14) / 55 = 7.58/55 over library-size denominators. The production code is unchanged; the derivation is recorded at the assertion. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
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 2 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 |
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. |
Owner
Author
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7bff9666d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| u.residual_count, Epistemic.cloud_size_by_residual(u.residual_count), | ||
| u.id, u.name, u.rank, u.direct_count, own_count, freq, | ||
| own_residual, Epistemic.cloud_size_by_residual(own_residual), | ||
| u.branch_length, u.support, sub_pd, u.dominant_taxon, u.taxon_purity, mono, |
There was a problem hiding this comment.
When a move detaches an internal clade's only child, counts_as_tip is false and the node is correctly assigned zero reads, but the rebuilt leaf still copies its former dominant_taxon, taxon_purity, and potentially monophyletic values. The counterfactual response therefore presents an empty clade as having the taxonomy of the departed subtree, despite the function's promise to recompute taxonomic summaries; clear these fields when the vacated node no longer counts as a tip.
Useful? React with 👍 / 👎.
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.
Follow-up to #239, which was merged at commit
a10aa39while its Julia job was still running. That job finished with exactly one failure (5715 pass): the.jplaceweighted-UniFrac assertion added on #235 pinned0.15636, a constant that was never exercised by a completed Julia run on that branch (its post-merge runs were cancelled/skipped).CI evaluates
0.1378181818181818, which is the branch-by-branch value — (0.02×24 + 0.05×24 + 0.02×10 + 0.15×10 + 0.30×|34−20|) / 55 = 7.58/55 over library-size denominators — so the production code is correct and only the expectation changes, with the derivation recorded at the assertion. No production code is touched.This also confirms the rest of #239 under CI: the new emptied-clade tip-count regression test (root purity 0.5 vs 0.6) passed in that run.
🤖 Generated with Claude Code