Repository navigation
Fix validated-statistics run summaries 500 (invalid GROUP BY aggregation) - #218
Merged
Merged
Conversation
#193) Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…nifold-webui # Conflicts: # README.md # config/defaults/pipeline.yml # frontend/src/api/types.ts # frontend/src/components/PipelineStages.tsx # frontend/src/components/differentialTable.ts # src/analysis/differential.jl Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
The squash merges of #196-#207 left main unable to build: PR-branch conflict resolutions overwrote each other's differential method registry, dropped the compositional include and cladistics routes, duplicated the clade-cumulus include and frontend panel import, and corrupted _differential_settings_error. - MetaManifold.jl: restore analysis/compositional.jl, include clade_cumulus.jl once; server.jl includes routes/cladistics.jl and routes/clade_cumulus.jl. - differential.jl: rebuild the 9-method registry and DifferentialConfig union (mn/dm optimiser + mc_samples), restore ancom_bc/aldex2/ songbird and multinomial dispatch branches alongside the two-part models; routes build the union config; config validation regains the mn/dm cross-key rules. - Frontend: union DifferentialMethod/config/diagnostics types, restore cladistics client imports, alias the cladistics panel so it no longer collides with the clade-cumulus panel, restore per-method CSV config lines and PipelineStages entries for all nine methods. - Tests that pinned pre-union method lists and the 11-column CSV are updated to the union reality. Frontend: tsc clean, bun test 87/87, production build passes. Julia: exercised by CI (no Julia runtime in the authoring sandbox). Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…nifold-webui # Conflicts: # config/defaults/pipeline.yml # frontend/src/api/types.ts # frontend/src/components/PipelineStages.test.ts # frontend/src/components/PipelineStages.tsx # frontend/src/components/differentialTable.ts # frontend/src/views/StudyView.tsx # src/analysis/differential.jl # src/server/routes/analysis.jl # test/unit/test_differential.jl Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…nifold-webui # Conflicts: # README.md Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…nifold-webui Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…d GROUP BY SQL
_validated_duckdb_counts selected bare count columns beside
GROUP BY <feature>, which DuckDB rejects with a BinderError, so
POST /studies/{study}/runs/{run}/statistics/summaries returned 500
and step 'Run validated-statistics tests first' failed on every
branch. Fetch rows ordered by feature and sum rows sharing a
feature in Julia BigInt instead: SQL SUM over HUGEINT counts would
overflow exactly the values this path exists to keep exact
(the test sums 2^126 + 2^126).
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. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 5
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. |
…res; dedupe cladistics routes - exact_summaries _summarise returned an exact 0 for a zero-count feature; the storage contract (and both test assertions) define an unobserved feature's proportion as undefined (nothing), never a substituted zero. summary_to_display renders it as such instead of calling exact_value(nothing). - server.jl included routes/cladistics.jl twice (parallel repair residue), re-registering every cladistics route. - test_validated_statistics: the integral-float token case cannot be expressed through a Dict round-trip (JSON3.write(1.0) emits an integer token), so 1.0 joins 1e0 as raw-body cases; the server-side rejection they exercise is unchanged. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…ependent reference computes The handwritten assertions expected nothing for a zero-count feature in a positive-total sample, contradicting both the implementation (since #199) and the independent Python fractions verifier, which computes Fraction(0, total) = 0/1. Only a zero total leaves proportions undefined. Aligns the two assertions; implementation reverted to the plain exact_relative_abundance call. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…equest text JSON3 collapses integral float tokens (1.0, 1e0) to integers when parsing, so the parsed-body check in _json_exact_count can never see them and the endpoint accepted them with 200. _reject_float_count_tokens scans the raw body to the top-level counts member and refuses any numeric token that is not a plain integer token (strings skipped and validated separately after parsing), restoring the fail-closed contract the tests assert. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…ute loading (#222) 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_queries` read `QueryCandidate.weight/.distal_length/.pendant_length`; the fields are `like_weight/distal_len/pendant_len` (FieldError on any valid graft). - `PLACEMENT_WEIGHT_SUM_TOL = 1e-5` marked 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). - The Newick label reader consumed `[&&NHX:B=92]` after `)` without capturing it; capture refs are threaded through `read_label_and_quoted`. - The sources route enumerated views via `SHOW TABLES` and then could not bind a session-registered DataFrame view on a fresh connection (500); it now uses `duckdb_tables()`. - `_load_run_table_df` rejected JSON3 `SubString` table names; arguments normalise to `String` at the boundary. - The FastTree mock in tests addressed `PH.Tools`, but `Phylogeny` imports only selected names from `Tools`; it now addresses `MetaManifold.Tools`. Julia evidence is CI on this PR (no Julia runtime in the authoring sandbox). --------- 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>
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:
mainCI still fails at 'Run validated-statistics tests first'.Root cause (reproduced against DuckDB locally):
_validated_duckdb_countsselected bare count columns besideGROUP BY <feature>:so
POST /api/v1/studies/{study}/runs/{run}/statistics/summariesreturned 500 andtest_validated_statistics.jl:180failed on every branch.Fix: fetch rows
ORDER BY featureand sum rows sharing a feature in JuliaBigInt. Aggregating in SQL is not an option:SUMover HUGEINT counts overflows exactly the values this path exists to keep exact (the test sums 2^126 + 2^126). Per-cell missing/negative/non-integer validation is unchanged; the cell budget is now checked on aggregated features.Julia evidence is CI on this PR (no Julia runtime in the authoring sandbox; the SQL failure itself was reproduced with Python duckdb 2.0.0).