Repository navigation
fix(analysis): read R's NA as missing so parametric fits run - #66
Merged
Merged
Conversation
arena-ai-coding-agent
Bot
requested a review
from hyperpolymath
as a code owner
September 25, 2026 22:01
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
hyperpolymath
approved these changes
Sep 25, 2026
hyperpolymath
added a commit
that referenced
this pull request
Sep 25, 2026
## Why After #66, the fits run. Run 36194648664 shows the remaining 19 failures and 6 errors. They all come from one fixture in `test/unit/test_estimation.jl`: two of its three features fail inside `MASS::glm.nb` (`n_tested = 1`, `n_failed = 2`, status `partial`). The fixture is **under-dispersed**: its variance is below its mean in every group block (taxon 1, group A: mean 10.2, variance 3.1; taxon 2, group B: mean 30.3, variance 1.5). Under a negative binomial model, variance = μ + μ²/θ ≥ μ, so the maximum-likelihood θ for this data is infinite. A direct likelihood fit of the old table gives θ = 2e8 to 2e17 for every taxon under both offsets. `theta.ml` then stops at its iteration limit, and the estimator marks the fit `failed`. **That is the right behaviour**, and this PR does not change it. ## What - `counts_effect` is replaced by a table with the same written-in group means (10→40, 30→30, 50→5). Each block has a variance near μ + μ²/6, all counts are ≥ 1 (so RLE is defined), and the numbers are deterministic literals. - A comment explains why the fixture has to be over-dispersed and records the outside-R check. - `CHANGELOG.md` entry. ## Evidence (outside R: there is no R/Julia in the authoring sandbox) Two independent NB2 maximum-likelihood fits (a direct scipy likelihood and statsmodels `NegativeBinomial`) agree to 4 decimals, converge, and raise no warnings. The table uses the two offsets the tests use: | taxon | offset | slope | log2FC | θ | p | |---|---|---|---|---|---| | t_up | log lib size | 1.4986 | 2.16 | 5.98 | 4e-14 | | t_flat | log lib size | 0.1578 | 0.23 | 7.63 | 0.34 | | t_down | log lib size | −2.0470 | −2.95 | 11.97 | 4e-29 | | t_up | RLE | 1.5451 | 2.23 | 3.52 | 2e-10 | | t_flat | RLE | 0.2378 | 0.34 | 14.89 | 0.07 | | t_down | RLE | −2.0510 | −2.96 | 2.99 | 1e-13 | Every assertion in the testsets holds against these numbers: `estimate ≈ log 4 ± 0.5`, `log2FC ≈ 2 ± 0.8`, `|flat| < 0.6`, `down < 0`, `p(up) < 0.01`. The parity with `glm.nb` itself is what this PR's CI run checks. Co-authored-by: hyperpolymath <6759885+hyperpolymath@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Sep 25, 2026
…salvageable delta of #71) (#72) <!-- SPDX-License-Identifier: CC-BY-SA-4.0 --> ## Summary Re-anchors **the one genuinely missing piece** of PR #71 onto `main`, and documents why everything else in that PR is already here in newer form. ## Why PR #71 cannot merge (measured, 2026-09-26) 1. **Wrong lineage → `mergeable: CONFLICTING` (DIRTY).** The PR branch is built on the **upstream parent's history** (`JoshuaJewell:main` = `ecefb1c` is an ancestor; the PR = upstream + 3 commits). `main` here shares only the **initial commit** `7884553` with it — the exact fork↔upstream divergence documented in `docs/integration/conflict-map-2026-09-25.md`. A test merge surfaces **38 conflicting files** (add/add: ci.yml, ui.yml, package.json, MetaManifold.jl, Execution.jl, tests, docs, binary `bun.lock`). 2. **Content is ~99% superseded.** Of the PR's 319 changed files, **281 are byte-identical with `main` already** (the migration was re-anchored earlier via `0e61f4f` and siblings). Net of `main`, the PR branch has only **+398 lines**, and after filtering to lines the PR itself authored that `main` truly lacks: - **`src/analysis/Execution.jl` (124 lines): stub/mock code** (“Mock p-value”, “Real implementation would call R…”) — replaced on `main` by the real statistics layer (#57–#66). - **Schema/doc notes “TSS/CSS/RSS deferred alias”** — obsolete: `main` implemented exact TSS/CSS/RSS offsets in #61. - **CI action pins** — older than `main`'s (Dependabot keeps `main` at checkout v7.0.1 / setup-julia v3.0.2 …). - **Escape-key dialog handlers** — superseded by native `<dialog>` with `showModal()` (#32), which supplies Escape/role/focus-trap natively. - **`lint_source.jl` undeclared-deps, Justfile bootstrap/setup-full, coupling test** — all present on `main` in deliberately newer forms. - **The 2 CodeQL alerts (“Workflow does not contain permissions”)** — the author already fixed these on their branch (`6b481a3`, `eab8ea0`), and *that* piece is what `main` was still missing. ## Changes - `.github/workflows/ci.yml`: top-level `permissions: contents: read` plus job-level blocks on `repo-hygiene` and `test` (`cicd-squabbler` already declares its own). Mirrors `ui.yml`, which already has a top-level block, and the fix landed on PR #71's branch. ## Recommendation **Close #71** in favour of this PR (its remaining unique content would regress `main`); carry any future application changes to `JoshuaJewell` per the PR template's Base check. ## Testing - `npx js-yaml .github/workflows/ci.yml` parses; `permissions` present at workflow level and on all jobs. - `scripts/check-spdx.sh` OK (291 files), `scripts/check-format.sh` OK (339 files), `scripts/check-lint.sh` advisory (bun not installed in sandbox). Co-authored-by: hyperpolymath <6759885+hyperpolymath@users.noreply.github.com> 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.
Why
With #65's annotations, the red Julia job on
mainis readable for the first time. Run 36193210557 names 11 failures and 20 errors, all intest/unit/test_estimation.jl. The Scaling suite (#16) passes. There are three causes:not_run. R writes the fits withwrite.csv(..., na = "NA"), and Julia read them withCSV.File(path), whose defaultmissingstringis"". Any numeric column containing anNAwas read asString31, and_numthen threwMethodError: no method matching isfinite(::String31). The catch-all turned that intostatus = :not_run, which produced the cascade ofKeyErrors (n_tested,n_failed,correction,t_up) and0 == 3results in five testsets.glmGamPoinever reached its feat(analysis): Advanced zero handling — glmGamPoi dispersion, Bayesian multiplicative replacement, multiplicative replacement with delta #21 refusal.AdvancedConfiglower-casesdispersion_methodand then checks it againstVALID_DISPERSION_METHODS, which spells itglmGamPoi. Sonb_config(dispersion = "glmGamPoi")was refused as an unknown name. This is the same class of bug as thetss/TSSone fixed in fix(analysis): method names compare in lower case; offsets wired in #62.estimation.jlandExecution.jlquoted the removed code literally (hash(taxon_id)).What
estimation.jl:R_NA_STRINGS = ["NA", ""]is passed asmissingstringto bothCSV.Filereads._refused_dispersionlooks upREFUSED_DISPERSIONcase-insensitively, and the error message uses the canonical name.AnalysisConfig.jl: the dispersion compare is now lower-case againstlowercase.(VALID_DISPERSION_METHODS).test_estimation.jl: three extra spellings (glmgampoi,GLMGAMPOI,glmGamPoi) must each hit the feat(analysis): Advanced zero handling — glmGamPoi dispersion, Bayesian multiplicative replacement, multiplicative replacement with delta #21 refusal.CHANGELOG.mdentry.Verified / not verified
reasonstring in the annotation. Once the fits run, some numeric assertions that were never reached may surface in turn. If so, the annotations will name them.