Repository navigation
Fix CladeCumulus testset: field names, NHX capture, LWR tolerance, route loading - #222
Merged
Merged
Conversation
…rance, route table loading The CladeCumulus testset (the last red testset in the Julia suite: 817 pass / 3 fail / 14 error, all inside it) shipped in #207 with defects that the long-broken build had masked: - _graft_jplace_queries read QueryCandidate.weight/.distal_length/ .pendant_length; the struct fields are like_weight/distal_len/ pendant_len (FieldError on any valid jplace graft). - A single retained placement at LWR 0.98 was marked undecided: PLACEMENT_WEIGHT_SUM_TOL=1e-5 rejects truncated candidate lists whose retained ratios legitimately sum to slightly less than 1. Tolerance is now 0.05; a 0.40 sum stays undecided. - The Newick label reader consumed an NHX comment after ')' without capturing it, dropping [&&NHX:B=92] support; capture refs are now threaded through read_label_and_quoted. - The sources route listed views via SHOW TABLES and then failed to bind a session-registered DataFrame view on a fresh connection (500); it now enumerates real tables via duckdb_tables(). - _load_run_table_df rejected the SubString table names JSON3 returns; arguments are normalised to String at the boundary. - Test used PH.Tools, but Phylogeny imports only selected names from Tools; the mock-tool override now addresses MetaManifold.Tools. 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. |
|
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 48 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 |
Fork governance
🟣 critical · commit
Hypatia scan: 133 finding(s)
panic-attack assail: 19 finding(s)
pons asinorum: 10 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. |
![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: b6ab9325b8
ℹ️ 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".
| # like-weight ratios can legitimately sum to slightly less than 1 (a single | ||
| # retained placement at 0.98 is complete evidence). Sums far from 1 indicate | ||
| # a malformed or partial record and stay undecided. | ||
| const PLACEMENT_WEIGHT_SUM_TOL = 0.05 |
There was a problem hiding this comment.
When candidate LWRs total between 1.0 and 1.05 (for example, 0.60 + 0.44), the existing abs(weight_sum - 1.0) check now marks the query valid. Truncating a candidate list can only lower its retained mass, so the widened tolerance should apply only below 1.0; otherwise malformed overweight records are accepted and subsequently normalized into apparently valid placement evidence.
Useful? React with 👍 / 👎.
| for q in jd.queries | ||
| q.status === :valid && !isempty(q.candidates) || continue | ||
| total_w = sum((c.weight for c in q.candidates); init = 0.0) | ||
| total_w = sum((c.like_weight for c in q.candidates); init = 0.0) |
There was a problem hiding this comment.
For a CladeCumulus POST using a valid .jplace file and a table with sample columns, this fix lets _graft_jplace_queries complete, but the ensuing diversity block in src/server/routes/clade_cumulus.jl:279-285 still reads c.weight and cand.weight. QueryCandidate has only like_weight, so the advertised HTTP flow still raises a FieldError and returns a 500 instead of the analysis response.
Useful? React with 👍 / 👎.
Comment on lines
+70
to
+71
| study = String(study); run = String(run); table = String(table) | ||
| group = isnothing(group) ? nothing : String(group) |
There was a problem hiding this comment.
When either POST route uses treeSource: "study_tree", tree_file = strip(string(...)) in _resolve_tree_inputs_from_request is a SubString, while _valid_tree_file and _tree_format in routes/trees.jl accept only String. Normalizing the table arguments only inside this loader allows the request to advance to that next MethodError, so existing .nwk and .jplace selections still fail before the tree is read; normalize tree_file as well or broaden those helper signatures.
Useful? React with 👍 / 👎.
src/core/doi.jl ended with 'end # module DOI' but never opened the module, so including it into MetaManifold failed to parse (ParseError at doi.jl:681) and no Julia test could even load the package. Routes and tests address MetaManifold.DOI, so the file is a submodule whose opener was lost in an earlier repair. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
Bring in #222's DOI/module and CladeCumulus repairs, retain valid source-table enumeration, and reject overweight jplace queries while allowing truncated weight shortfalls. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
This was referenced Oct 23, 2026
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
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.
hyperpolymath
pushed a commit
that referenced
this pull request
Oct 23, 2026
Builds on #222, which fixed the graft query fields but left three sites: - `_graft_jplace_queries` still constructed `PhyloNode(..., u.edge_num, ...)` and read `u.edge_num` when matching placements to edges. The field is `edge_number`, so any graft threw `FieldError` before the tree was rebuilt ("Got exception outside of a @test" — no test line, which is why it read as an unrelated suite failure). - The diversity block in the route still summed `c.weight` over jplace candidates. `QueryCandidate` carries `like_weight`; reading `.weight` threw inside the handler, and with `show_errors=false` the POST answered a bare 500, so every assertion on the combined-mode payload failed with `KeyError: key :root not found`. - `routes/cladistics.jl` was included twice in server.jl, registering every /cladistics route twice. `cand.edge_num` stays: that is the `QueryCandidate` field. The undirected edge struct's `edge_no` and its `weight` are separate types. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…d syntax checks Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
## What this is #217 (`fix(stats): aggregate DuckDB counts exactly`) has been `CONFLICTING` since main moved: #218 fixed the same run-summaries 500 independently, #210 repaired the differential/analysis modules, #220 removed the Evidence UI, and #222 repaired the CladeCumulus tree/route code. This branch merges the current main into #217's head, resolves every conflict, and repairs the two main-line defects that were failing the required `Julia tests` check on *both* branches. It contains #217's content in full, so merging it lands #217. ## Conflict resolution (4 files) - `src/server/routes/validated_statistics.jl` — **the PR's implementation is kept unchanged.** Main's #218 kept a `GROUP BY`-free `ORDER BY … LIMIT MAX_CELLS+1` window and scanned only the `counts` member for float tokens; #217 streams the raw rows, bounds features/cells *as rows arrive*, and scans the whole direct body. Nothing main added after 7bb716d was lost: the two versions differed only in those four hunks. - `test/unit/test_validated_statistics.jl` — union. Main's `(1.5, -1, 2^53+1)` Dict cases and its `1.0`/`1e0` raw-token loop are kept next to #217's byte-backed `Vector{UInt8}` cases and its `round_digits: 8.0` rejection, so both sides' contracts are now tested. - `test/unit/test_exact_summaries.jl` — main's wording; same assertions. - `.github/workflows/ci.yml` — main's log path/title with #217's ordering that runs the validated-statistics suite first. The CladeCumulus merge takes **main's** #222 implementation (real tables only via `duckdb_tables()`, `SubString` normalisation at the request boundary, NHX capture, `PLACEMENT_WEIGHT_SUM_TOL = 0.05`) and keeps the two repairs #222 still lacks: `PhyloNode.edge_number` in `_graft_jplace_queries` (main reads a field that does not exist) and the connecting branch in `_reaggregate_after_move`, so a subtree PD agrees with `clade_faith_pd`. ## Two main-line fixes needed to get the required checks green 1. **`UndefVarError: erfc`** — `CompositionalModels._wald` called `erfc` without importing or defining it, so every multinomial / Dirichlet- multinomial fit raised on any tree where it could load. The module states it carries its own special functions so the models add no dependency, so `_erfc` is added in the same style: `erfc(t) = Q(1/2, t²)`, power series below the switch, continued fraction above it (modified Lentz). 2. **`DifferentialConfig(…, min_prevalence=1.0)`** — the test passed `min_prevalence` as a keyword; it is positional in the constructor and in the route that builds it. Fixed in the test. ## Validation - DuckDB (independent of Julia): the old grouped query fails with `column "s1" must appear in the GROUP BY clause…`; the streamed query returns `G → [3, 2^127]`, `H → [4, 1]`, and the `prefix=s2` + `Pident ≥ 99` filter returns `s2 → 2^126`, exactly the values the tests assert. - The integer-token scanner was ported line-for-line and run over the test bodies: `1.0`, `1e0`, `3.0e-1` and `round_digits: 8.0` are refused; integer tokens, quoted `"1.0"`, escapes and digits inside strings are accepted; the run route (which legitimately takes `Pident: 99.0`) is not scanned. - `_erfc` was checked against libm's `erfc` on `t ∈ [0, 30]`: worst relative error 1.5e-13, and the two-sided tail reproduces this file's p-value references to 1.5e-12 (the test asserts `rtol = 1e-6`). - All 122 `src/**.jl` and `test/**.jl` files parse with a Julia grammar; the three files that still show grammar nodes (`remote_exec.jl`, `occupancy_disagreement_test.jl`, `test_determinism.jl`) are untouched here. - `git diff --check` clean; `.github/workflows/ci.yml` parses as YAML. - Julia itself is not installable in this sandbox (the toolchain download host is blocked), so the pinned suite has to be the authority: this PR exists to give CI the merged tree to run on. ## Relationship to #217 Merging this lands #217's content. To make #217 itself mergeable instead, its head must be advanced to this branch; that is a single command for someone who can push to it: ``` git push origin arena/a8928088-metamanifold-webui:arena/dd990535-metamanifold-webui ``` --------- Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
Main landed #226, which contains equivalents of this branch's _erfc implementation, min_prevalence test fix, edge_number graft fix and stem-inclusive subtree PD, plus #217's validated-statistics implementation and the CLADE_SV rename that removes the Main.SV const collision. Conflicts resolved in favour of main for those; this branch keeps its unique additions: AbstractString tree helpers and the structured clade_cumulus_failed catch-all with the like_weight diversity fixes. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
Follow-up to #222 (merged before this commit arrived). The Julia suite on #222 reached 825 pass / 1 fail / 9 error, all remaining in CladeCumulus, from two root causes: - The jplace graft walk read `u.edge_num`/`cur.edge_num`; `PhyloNode`'s field is `edge_number` (`QueryCandidate.edge_num` is a different, legitimate field). Grafting any valid placement threw FieldError. - `_valid_tree_file` only accepted `::String`, but `strip()` of a request field yields a `SubString`, so POST clade-cumulus with `treeSource: study_tree` returned 500. It now accepts `AbstractString`. Julia evidence is CI on this PR. --------- Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up to #214/#218. With the frontend, differential and validated-statistics suites green, the Julia suite stands at 817 pass / 3 fail / 14 error — every remaining failure inside the CladeCumulus testset from #207, masked until now by the broken build.
_graft_jplace_queriesreadQueryCandidate.weight/.distal_length/.pendant_length; the fields arelike_weight/distal_len/pendant_len(FieldError on any valid graft).PLACEMENT_WEIGHT_SUM_TOL = 1e-5marked a single retained placement at LWR 0.98 undecided; truncated candidate lists legitimately sum to slightly less than 1, so tolerance is 0.05 (0.40 still undecided).[&&NHX:B=92]after)without capturing it; capture refs are threaded throughread_label_and_quoted.SHOW TABLESand then could not bind a session-registered DataFrame view on a fresh connection (500); it now usesduckdb_tables()._load_run_table_dfrejected JSON3SubStringtable names; arguments normalise toStringat the boundary.PH.Tools, butPhylogenyimports only selected names fromTools; it now addressesMetaManifold.Tools.Julia evidence is CI on this PR (no Julia runtime in the authoring sandbox).