Repository navigation
feat(differential): add optional glmGamPoi dispersion trend through RCall - #62
Open
hyperpolymath wants to merge 3 commits into
Open
hyperpolymath wants to merge 3 commits into
hyperpolymath wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
hyperpolymath
marked this pull request as ready for review
October 18, 2026 09:26
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. |
Contributor
Author
|
this is now one Verified, single-parent commit |
The shrunken dispersion trend that analysis.differential.dispersion: glmGamPoi (issue #37) will use is fitted by glmGamPoi::glm_gp, so the package must be in the restored library and every list that enumerates it. renv.lock gains glmGamPoi 1.22.0 and its 11-package closure on R 4.5.0 (Bioc 3.22): assorthead 1.4.0, beachmat 2.26.0, DelayedMatrixStats 1.32.0, HDF5Array 1.38.0, h5mread 1.2.1, RcppArmadillo 15.6.2-1, rhdf5 2.54.1, rhdf5filters 1.22.0, Rhdf5lib 1.32.0, SingleCellExperiment 1.32.0 and sparseMatrixStats 1.22.0; DelayedArray, MatrixGenerics and SummarizedExperiment were already present and are unchanged. R/_renv_dependencies.R loads glmGamPoi, the runtime probe and install.jl's availability check require it, and both CI R-library installations install it, so the pinned closure and the required-package lists stay in step. Dependency plumbing only; no behavior change. Prepares for #37. Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
Implement #37. analysis.differential.dispersion chooses the fits and the test for nb_glm. The default glm_nb keeps MASS::glm.nb with its maximum-likelihood theta per taxon and a Wald test of the group coefficient, so #24's results are unchanged. The alternative glmGamPoi fits one glmGamPoi::glm_gp over the taxa passing min_prevalence with overdispersion shrinkage and reports glmGamPoi::test_de's quasi-likelihood F-test of the group contrast -- its own published inference -- with the returned log2 fold changes and their standard errors converted to the natural-log scale so every method reports one effect scale. The reported dispersion is the inverse of the shrunken quasi-likelihood dispersion the test used. A glm_gp or test_de error fails every taxon; a zero shrunken dispersion marks only its own taxon as a Poisson boundary and withholds inference there, and an unusable dispersion fails only its own taxon. Missing glmGamPoi raises GlmGamPoiUnavailable (HTTP 503 on the analysis route), never a silent fallback to another estimator or test. glmGamPoi is refused for clr_welch; the pair rule lives in DifferentialConfig and is reported at analysis time, while the config routes keep their ordinary per-key validation with the dispersion spellings registered alongside the other enums. The frontend offers the dispersion enum in method settings, and the tests compare the glmGamPoi path against direct glm_gp + test_de calls in R -- p-values, F statistics and log fold changes -- failing (not skipping) in CI when glmGamPoi is missing. Closes #37. Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
hyperpolymath
force-pushed
the
upstream/glmgampoi-r
branch
from
October 18, 2026 21:23
22a09ea to
efe1df1
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
|
Thanks, this is close. Two fixes before I merge: In the glmGamPoi path, Also please update the PR description: it still describes the trend + Wald approach, the config rewrite and a single commit. |
…rast (#192) ## Change and reason Prepares the next push to upstream [JoshuaJewell#62](JoshuaJewell#62), the glmGamPoi dispersion option for JoshuaJewell#37. The branch is upstream PR 62's reviewed head (`efe1df1`) plus three new commits, so the update fast-forwards. Under upstream CONTRIBUTING item 8, nothing is rebased once review has started. 1. **Report glm_gp's negative-binomial overdispersion as theta.** Your `wip/pr62-theta` commit, cherry-picked unchanged. It covers both of Joshua's code requests from Oct 19 (theta is `1 / overdispersions`; the Poisson boundary is read from it). 2. **Pass the glmGamPoi contrast to test_de as numeric weights.** `glmGamPoi::test_de` parses and evaluates a text contrast as R code. The contrast label is built from run, group and sub-group names joined by `/`, and those names may contain `-`. So the glmGamPoi path failed every taxon for comparisons such as `run-A` vs `run-B` or `grpA/run_1` vs `grpB/run_2`, and label text that reads as R code was executed. The commit adds a regression test that covers both dispersions. 3. **State what glmGamPoi's theta, standard error and boundary mean.** These are precision fixes, checked against the glmGamPoi 1.22.0 source: - `overdispersions` is the per-taxon estimate before shrinkage. The coefficients and the F-test use the cross-taxon trend, and only `lfc_se` reads `overdispersions`. - The boundary note no longer says the fit is Poisson. - The README now lists `dispersion` with the other study-scoped settings. **Do not merge this PR on GitHub.** Merging would put unsigned commits, or a merge commit, onto upstream PR 62. Re-sign on your machine and push instead: ```sh git fetch origin claude/elegant-rubin-z92rx4 upstream/glmgampoi-r git switch -c pr62-next origin/claude/elegant-rubin-z92rx4 git rebase --exec 'git commit --amend --no-edit -S' efe1df1 # re-signs only the 3 new commits git log --format='%h %G? %s' efe1df1..HEAD # expect G or E, never N git push origin pr62-next:upstream/glmgampoi-r # fast-forward: no --force needed ``` When Joshua asks for a rebase (upstream `main` is now `ce2c645`), the five-commit stack rebases cleanly onto it. That was checked in a scratch worktree. ## Validation Environment: Julia 1.12.5 (the CI pin, from the official image), R 4.3.3, MASS 7.3-60.0.1, glmGamPoi 1.14.3, bun 1.4.2. glmGamPoi 1.14.3 lacks `compute_lfc_se`, so 1.22.0's pure-R `test_de`, `test_de_q` and `predict.glmGamPoi` were loaded over it. Diffing the 1.14.0 and 1.22.0 sources shows no other change on these paths. Upstream CI runs the real pins (R 4.5, glmGamPoi 1.22.0). - **`CI=true julia --project=. test/runtests.jl test_differential.jl`:** - **head:** 253 passed, 0 failed, 5 skipped. The skips are `clr_welch` tests that need zCompositions. `CI=true` makes a missing glmGamPoi fail rather than skip, so every glmGamPoi test ran. - **commits 1, 2 and 3 on their own:** 202, 253 and 253 passed, 0 failed. - **the new label test on the pre-fix code:** 5 failures, as expected. The four label sets fail every taxon, and the R-code label ran. - **Label experiments** (the `fit_nb` R block, run directly): - **Before:** `run-B`, `grpB/run_2` and `2024-02` failed every taxon, and a label of `[file.create(...)]` created the file. - **After:** every label set gives rows identical to `A`/`B`, the file is not created, and `glm_nb` is unchanged. - **Null calibration** (share of p < 0.05, binomial SE about 0.003): | samples per group | glm_nb (Wald) | glmGamPoi (QL F) | |---|---|---| | 4 | 0.132 | 0.046 | | 12 | 0.069 | 0.048 | - **Frontend:** `bun test src` 39 pass; typecheck clean; build succeeds. Not run: the integration and server suites, the DADA2 taxonomy and phylogeny paths, and the `clr_welch` tests (zCompositions unavailable; they skip loudly). ## Scope No lockfile change. The upstream PR description still has to be replaced; Joshua's third request asks for that. A draft follows, together with the summary comment for upstream issue 37. <details><summary>Draft replacement description for upstream PR 62</summary> ~~~~markdown ## Change and reason Adds an optional `analysis.differential.dispersion` setting for the `nb_glm` method (issue #37). `glm_nb` stays the default and is unchanged: `MASS::glm.nb` fits each taxon with its own maximum-likelihood theta, and the group coefficient gets a Wald test. `glmGamPoi` fits one `glmGamPoi::glm_gp` across the taxa that pass `min_prevalence`, with `overdispersion_shrinkage = TRUE`. It reports `glmGamPoi::test_de`'s quasi-likelihood F-test of the group contrast (Lund et al., 2012), which is glmGamPoi's own published inference. The trend-plus-Wald design in the first version of this PR has been replaced. In that path: - **estimate and se**: `lfc` and `lfc_se`, converted from log2 to the natural-log scale. `lfc_se` comes from `predict()` and does not feed the p-value. - **statistic and pvalue**: the F statistic and its p-value. The F-test refits the reduced model at the cross-taxon dispersion trend and divides by the shrunken quasi-likelihood dispersion. - **dispersion_theta**: `1 / overdispersions`. This is glm_gp's negative-binomial overdispersion, the taxon's own estimate before shrinkage, and the same parameter `MASS::glm.nb`'s theta inverts (requested in review). - **Poisson boundary**: based on that overdispersion (requested in review). A zero estimate marks the row `boundary` and keeps its statistics, since the test does not read that estimate. A negative or non-finite estimate fails only its own taxon. - **contrast**: passed to `test_de` as numeric weights on the design's columns. `test_de` parses and evaluates a text contrast as R code. Before this change, a contrast label containing `-` or `/` (e.g. `run-B`, or `grpB/run_2`, which the app builds when conditions differ by group and run) made every taxon fail. A label that reads as R code was executed. - **failures**: a missing glmGamPoi raises `GlmGamPoiUnavailable` (HTTP 503). A `glm_gp` or `test_de` error fails every taxon, with no fallback to `glm_nb`. `glmGamPoi` is refused for `clr_welch`. Config and UI: the config editor and `PATCH /api/v1/config` accept the new enum (`glm_nb`, `glmGamPoi`). The constructor refuses an invalid method/dispersion pair when the analysis runs (HTTP 400 `invalid_config`). There is no other config rewrite; the first version's description wrongly claimed one. The README lists the option with the other study-scoped settings. Commits, oldest first (the last three are new since your review): 1. `02ee915` Add glmGamPoi to the pinned R dependencies 2. `efe1df1` Add glmGamPoi dispersion option to nb_glm differential abundance 3. Report glm_gp's negative-binomial overdispersion as theta (review: theta and boundary) 4. Pass the glmGamPoi contrast to test_de as numeric weights (labels with `-` or `/`; label text evaluated as R) 5. State what glmGamPoi's theta, standard error and boundary mean (comments, note text, README, result type) Closes #37. ## Validation Run in a cloud container with Julia 1.12.5 (the CI pin), R 4.3.3, MASS 7.3-60.0.1, glmGamPoi 1.14.3, and bun 1.4.2. glmGamPoi 1.14.3 lacks `compute_lfc_se`, so 1.22.0's `test_de`, `test_de_q` and `predict.glmGamPoi` (pure R) were loaded over it. Diffing the 1.14.0 and 1.22.0 sources shows no other change on these code paths: `glm_gp_impl`, `parse_contrast`, the overdispersion estimate and the quasi-likelihood shrinkage are the same. CI on this PR runs the pinned R 4.5 with glmGamPoi 1.22.0. - `CI=true julia --project=. test/runtests.jl test_differential.jl`: - **branch head:** 253 passed, 0 failed, 5 skipped. The skips are the `clr_welch` tests, which need zCompositions. Under `CI=true` a missing glmGamPoi fails rather than skips, so the glmGamPoi tests all ran. - **each new commit on its own:** 202, 253 and 253 passed, 0 failed. - **the new label test against the pre-fix code:** it fails as intended. Each of the four label sets fails every taxon, and the R-code label's code runs. - The `fit_nb` R block, run directly in R on the `effect` fixture: - **labels**: `run-A`/`run-B`, `grpA/run_1`/`grpB/run_2` and `2024-01`/`2024-02` failed every taxon before the fix. With it, every label set, including one containing R code, gives rows identical to `A`/`B`. - **injection**: a label of `[file.create(...)]` created the file before the fix and does not after it. - **glm_nb**: rows are unchanged. - **field check**: setting `overdispersions[1] <- 0` changes `lfc_se[1]` only; `pval`, `f_statistic` and `lfc` stay the same. - Null calibration through the same R block (no taxon differs; TSS offsets; dispersion falling with the mean). Share of p < 0.05: | samples per group | taxa tested | glm_nb (Wald) | glmGamPoi (QL F) | |---|---|---|---| | 4 | ~5,900 | 0.132 | 0.046 | | 12 | 4,000 | 0.069 | 0.048 | The binomial SE is about 0.003. - Frontend (`frontend/`): `bun test src` 39 pass; `bun run typecheck` clean; `bun run build` succeeds. Not run here: the integration suite (`--integration`, `--server`), the DADA2 taxonomy and phylogeny paths, and the `clr_welch` tests (zCompositions was not available in the container; they skip loudly). ## Scope Left out: a native Julia gamma-Poisson fit (#37 settled on R), zero-inflated and occupancy models, any change to the `glm_nb` default or its results, and any UI beyond the one enum. Lockfile: `renv.lock` adds glmGamPoi 1.22.0 and the eleven packages it needs that were not already pinned. Existing pins are unchanged. The option cannot run without them, and CI and `install.jl` check that glmGamPoi loads. Noted for a separate issue, not this PR: in the calibration above, the default `glm_nb` Wald test rejects too often at small n. ## Contribution checks - [x] I followed [CONTRIBUTING.md](https://github.com/JoshuaJewell/MetaManifold-WebUI/blob/main/CONTRIBUTING.md). - [ ] This branch starts at current upstream main and contains no merges from fork main. It starts at `029fa61` and contains no merges. Since review has started, it gains commits rather than being rebased (item 8). It rebases cleanly onto `ce2c645` whenever you ask. - [ ] Every commit is signed and shows as Verified. *(tick after re-signing)* - [ ] CI has run and passes. *(tick after the push)* - [x] Existing licensing, citation and acknowledgement text is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01HtyPfZ1MWBB94RawgzQgz7 ~~~~ </details> <details><summary>Draft summary comment for upstream issue 37</summary> ~~~~markdown **Summary for the record** **Question (Oct 15):** take glmGamPoi's dispersion shrinkage for `nb_glm` (a) in R through RCall, or (b) as a native Julia gamma-Poisson fit with R kept as the oracle? **Answer (@JoshuaJewell, Oct 17):** (a). "glmGamPoi stays R, since there is no reputable Julia equivalent." **What follows from that, in #62:** - **Option:** `analysis.differential.dispersion: glmGamPoi` is optional. The default stays `glm_nb` (`MASS::glm.nb`), and its results are unchanged. - **Test:** inference is glmGamPoi's own quasi-likelihood F-test (`glm_gp` + `test_de`), not the trend-plus-Wald plan from Oct 15. - **Review fixes:** theta is `1 / overdispersions` (glm_gp's NB overdispersion), and the Poisson boundary is read from it, as requested. The contrast is now passed as numeric weights. Before that, any label containing `-` or `/` (e.g. `grpB/run_2`) made every taxon fail, and label text was evaluated as R code. - **Dependencies:** `renv.lock` adds glmGamPoi 1.22.0 and the eleven packages it needs; existing pins are unchanged. - **Evidence:** in a null simulation, the share of p < 0.05 was: | samples per group | `glm_nb` (Wald) | glmGamPoi (QL F) | |---|---|---| | 4 | 13.2% | 4.6% | | 12 | 6.9% | 4.8% | The `glm_nb` excess at small n is outside this issue's scope and is worth its own issue. **Good to go.** The R/RCall route is settled, and #62 is ready for re-review once the three new commits are signed and pushed. ~~~~ </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01HtyPfZ1MWBB94RawgzQgz7)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Owner
|
Thanks, the fixes look good. Before I merge:
Separately, please open an issue for the |
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
2 participants
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.
Adds the optional
analysis.differential.dispersion: glmGamPoipath requested in #37, using the R implementation through RCall.mlremains the default and continues to use per-taxonMASS::glm.nb.For the alternative, one
glmGamPoi::glm_gpfit estimatesdispersion_trendacross taxa passing the prevalence filter. Each retained taxon then usesMASS::negative.binomial(theta = 1 / trend)with a fixed-scale Wald test. The returned method description and config help explicitly state that trend-estimation uncertainty is not propagated and this is not glmGamPoi's quasi-likelihood test. Missing packages return a clear error; failed or zero-dispersion fits do not silently fall back to ML.The existing configuration editor exposes the option. API PATCHes save one complete canonical YAML document; PATCH and DELETE validate the effective method/dispersion pair in affected inherited study/group/run cascades before publishing. Regressions cover direct R reference fits, invalid trends, missing-package HTTP 503, canonical round trips and inherited updates/deletions.
renv.lockadds glmGamPoi 1.22.0 and eleven required dependency records: DelayedMatrixStats, HDF5Array, RcppArmadillo, Rhdf5lib, SingleCellExperiment, assorthead, beachmat, h5mread, rhdf5, rhdf5filters and sparseMatrixStats. Every existing package record is unchanged. Dependency discovery, installer/startup checks and both CI verification lists include glmGamPoi.Validation on Julia 1.12.5, R 4.5.0 and bun 1.3.10:
CI_SKIP_TAXONOMY=1), and the phylogeny end-to-end test does not run when its external tools are absent; no coverage of those paths is claimed.Reproduce after restoring
renv.lock:This change adds no native Julia statistical replacement or new UI stack. The branch contains one Verified, single-parent commit directly on current main (
behind_by: 0at submission).Closes #37.