Repository navigation
fix(cladistics): load the DOI module and repair the CladeCumulus readers - #223
hyperpolymath wants to merge 5 commits into
Conversation
|
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 43 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. |
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: 6f7d5f5f37
ℹ️ 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".
| for (ci, cand) in enumerate(q.candidates) | ||
| tid = multi ? string(qname, "@edge", cand.edge_num, "#", ci) : qname | ||
| tip_weights[tid] = cand.weight / tw | ||
| tip_weights[tid] = cand.like_weight / tw |
There was a problem hiding this comment.
When the newly accepted single-candidate placement has an LWR between 0.5 and 1.0, tw equals that candidate's weight, so this division always produces 1.0. The /clade-cumulus diversity block therefore assigns all of the query's abundance to the reported edge even though the parser explicitly treats the remaining probability mass as unplaced, inflating that tip's contribution to weighted UniFrac and related abundance-based results. Preserve the raw LWR for this one-candidate case, with the remainder accounted for as unplaced, rather than renormalizing it away.
Useful? React with 👍 / 👎.
|
The conflicts here are with #222, which landed on The conflict git could not see. Two hunks conflicted textually and were trivial (the
#230 resolves to this PR's two-rule model: a multi-candidate record partitions the mass and must sum to one within Codex P1, preserve unplaced LWR mass. The finding is right, and it is a defect this PR introduced: before it, a single candidate had to carry nearly all the mass, so Everything else from this PR carries over unchanged: the Closing in favour of #230 once CI there is green — the review history is worth keeping here. |
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.
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>
…y `end` Both merge-damage classes that broke this branch were invisible to the tests that existed: - A duplicated `include` in server.jl registers every route in that file twice. A request test cannot see it: the second registration shadows the first and both answer. `test_routes.jl` now reads server.jl and asserts that each include path appears once and that both cladistics route files are present, then asserts that no route file declares the same method+path twice. - A dropped `module` opener leaves a bare `end`, which parses cleanly and only fails at lowering — so the "Parse Julia sources" gate passed while the whole suite died at include time. `scripts/check-julia-syntax.jl` now rejects a top-level `end` with the code unit it was found at. Also names `DuckDB` and `DBInterface` in routes/clade_cumulus.jl rather than inheriting them from results.jl, which happens to be included earlier. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
fc00d81 to
1a6a25a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a6a25a1eb
ℹ️ 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".
| included = String[] | ||
| for m in eachmatch(r"include\(joinpath\(@__DIR__((?:,\s*\"[^\"]+\")+)\)\)", | ||
| server_src) | ||
| push!(included, join([c[2:end-1] for c in |
There was a problem hiding this comment.
When this test processes any matching include, eachmatch yields a RegexMatch, not a string, so c[2:end-1] attempts unsupported range indexing on the match object and raises before any registration assertions run. Extract and slice c.match (or strip the quotes from it) instead so the unit suite can complete.
Useful? React with 👍 / 👎.
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…nst SV (temporary) Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
Superseded by #225 ( What
CI on One thing this branch found that the fix on |
Pull request was closed
main(8b76b71) cannot load the Julia package, and the CladeCumulus suite — which had never been reached before, because the run aborted earlier — reports 3 failures and 14 errors. This PR fixes the load failure, the route registration, and every CladeCumulus defect the suite can now see.1. The package does not load
src/core/doi.jlmodule DOIand leftend # module DOI"end" at top level) at include time, so every test file fails before it startssrc/server/server.jlroutes/cladistics.jlincluded twice/cladisticsroute registered twice (silently shadows itself)2. CladeCumulus, once the suite can run
routes/clade_cumulus.jlopened a DuckDB database without importingDuckDB/DBInterface. The sources endpoint answered 500, and every HTTP assertion downstream of it failed. Both modules are imported now, as the other route files already do.QueryCandidatehaslike_weight,distal_len,pendant_len;PhyloNodehasedge_number. The jplace graft path and the route's diversity block still usedweight,distal_length,pendant_lengthand.edge_num— fiveFieldErrors.parse_newicklost NHX support. The label reader consumed[&&NHX:B=92]before the call that records it, so) [&&NHX:B=92] :0.1parsed with supportnothing. The reader now takes the capture refs.parse_jplacerejected a legitimate single-candidate placement. Placement tools report a query placed at one site as a single row whoselike_weight_ratiois the share of the mass behind it; the rest is mass the tool did not place. A single candidate is now decisive at>= PLACEMENT_SINGLE_MIN_LWR(0.5); a multi-candidate placement must still partition the mass withinPLACEMENT_WEIGHT_SUM_TOL, and a single candidate below half staysundecidedwith the same reason string.PH.Tools, butPhylogenyimportstool_binselectively and never bindsTools. The registry belongs toMetaManifold.Tools.3. Guards against both merge-damage classes
scripts/check-julia-syntax.jlnow rejects a bareendat the top level. That file parses cleanly and fails only at lowering — exactly how the missingmodule DOIopener passed the "Parse Julia sources" gate while breaking the whole suite.test/unit/test_routes.jlasserts that no method+path pair is registered twice, and that both cladistics route files are present. A duplicate registration cannot be caught by a request test: the second registration simply shadows the first.Validation
Frontend:
npx tsc --noEmitclean,bun test srcunchanged. Julia is not available in this environment (no runtime, and the pinned download host is unreachable), so the Julia evidence is static plus CI: tree-sitter parses every touched file, a block-balance pass oversrc/andtest/unit/reports every file balanced, all route helper calls resolve, and no file is included or registered twice. I am following CI on this branch and will fix what it reports.