Repository navigation
Prepare upstream PR 62 review fixes: theta, label-safe glmGamPoi contrast - #192
Merged
Merged
Conversation
The glmGamPoi path reported 1 / ql_disp_shrunken as the dispersion theta, but that is the quasi-likelihood scale test_de divides by, not the negative-binomial parameter MASS::glm.nb's theta inverts. Read glm_gp's overdispersions element instead (variance = mu + mu^2 * overdispersion) and report its inverse, so dispersion_theta means one thing under both dispersion settings. The Poisson boundary moves with it: a taxon whose negative-binomial overdispersion is zero is marked boundary, as under glm_nb, and keeps its statistics, because the F-test's denominator is the shrunken quasi-likelihood dispersion and stays defined there. The earlier withholding was justified only by a zero quasi-likelihood scale, which this check no longer reads. A negative or non-finite overdispersion still fails only its own taxon. theta is nothing where it is unbounded. The direct R reference in the tests inverts gp$overdispersions, and the isolation test overrides that element, which test_de reads neither for its refit nor for its F ratio, so the test under comparison is unchanged. Requested in review on #62.
glmGamPoi::test_de parses a text contrast with rlang::parse_expr and
evaluates it, so the coefficient name built from the contrast label
reached R as code. Condition labels join group, run and sub-group
names with "/", and those names may carry "-", so comparing
"grpA/run_1" with "grpB/run_2", or "run-A" with "run-B", stopped
test_de and failed every taxon, while glm_nb fitted the same labels.
A label that reads as R code ran: with glmGamPoi 1.14.3 carrying the
1.22.0 test_de, a label of [file.create("<path>")] created the file
before test_de stopped.
The design matrix is now built once, and test_de receives the numeric
contrast that selects the contrast column, which it uses without
parsing. For labels that were already syntactic the rows are identical.
A test fits both dispersions under labels with "-", "/" and R code, and
requires the A/B results and that the code never ran.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HtyPfZ1MWBB94RawgzQgz7
With overdispersion_shrinkage = TRUE, glm_gp returns each taxon's own overdispersion estimate in `overdispersions` but refits the coefficients at the cross-taxon trend, and test_de refits the reduced model at that trend and divides by the shrunken quasi-likelihood dispersion; only predict(), which supplies lfc_se, reads `overdispersions`. The comments, the boundary note, the isolation test's comment, the README and the result type now say so: theta is the taxon's estimate before shrinkage, and a zero estimate leaves the test's statistics standing because the test used the trend, so the fit there is not Poisson. The README lists dispersion with the other study-scoped analysis.differential settings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtyPfZ1MWBB94RawgzQgz7
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
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 |
|
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. |
hyperpolymath
pushed a commit
that referenced
this pull request
Oct 20, 2026
The owner chose to replace the squash commit e28e832 on upstream/glmgampoi-r with #192's three commits, re-signed. Those commits are still reachable as refs/pull/192/head after the branch was deleted, so the entry gives the fetch, the re-sign from the reviewed head, an empty-diff check against e28e832, and a push whose lease names e28e832. The sequence was rehearsed in a scratch clone with a no-signature amend and `git push --dry-run`: three commits rewritten, no diff, lease accepted, nothing pushed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtyPfZ1MWBB94RawgzQgz7
hyperpolymath
added a commit
that referenced
this pull request
Oct 20, 2026
…h path (#194) ## Change and reason This is a fork-only note, not meant for upstream. It corrects `notes/commit-signing.md` (#191) in three places. 1. **A PR already under review.** The routine's `git rebase --exec ... upstream/main` replays every commit after upstream's tip. On a PR branch that started from an older `main`, that is itself a rebase, which upstream CONTRIBUTING item 8 allows only when Joshua asks. A new section re-signs only the commits added since the reviewed head, after checking that the branch contains that head, and pushes them as a fast-forward: ```sh git merge-base --is-ancestor <reviewed-head> HEAD && git rebase --exec 'git commit --amend --no-edit -S' <reviewed-head> git push origin <local>:<pr-branch> # no --force ``` Without the check, a branch that lacks the reviewed head would be replayed onto it, upstream commits included, and still push as a fast-forward. On `wip/pr62-theta` that would replay upstream's `78ffc2f` re-signed. 2. **Squash-merging the fork PR instead.** This is the path #192 took. GitHub signs the squash commit and the upstream PR moves forward, but: - the commit's message defaults to the fork PR's description, and it lands on Joshua's `main` unchanged (`e28e832` carries #192's whole description); - the prepared commits collapse into one; - the signature is GitHub's key, not one registered to your account. Upstream CONTRIBUTING item 3 asks for the latter, and has not been asked about this. 3. **Branch lists.** You chose to replace `e28e832` with #192's three commits, re-signed. They are still reachable as `refs/pull/192/head`: ```sh git fetch origin pull/192/head:pr62-next upstream/glmgampoi-r git switch pr62-next git merge-base --is-ancestor efe1df1 HEAD && git rebase --exec 'git commit --amend --no-edit -S' efe1df1 git diff --stat origin/upstream/glmgampoi-r HEAD # expect no output git push --force-with-lease=upstream/glmgampoi-r:e28e832fd52466f5192406025fe6b16d9fd99ae1 origin pr62-next:upstream/glmgampoi-r ``` `wip/pr62-theta` moves to a new "Superseded branches" section, kept rather than deleted. ## Validation All of this ran in scratch clones or worktrees, with a no-signature `--amend` standing in for `-S`, since this container has no key. - **Old routine** (`--exec ... upstream/main`) on upstream PR 62's stack: rewrote all five commits, the two reviewed ones included. - **New routine** (`--exec ... efe1df1`): rewrote the three new commits only, with the tree unchanged. - **Guard:** - on `wip/pr62-theta`, the check failed, the rebase did not run, and HEAD was unchanged; - on `refs/pull/192/head`, it passed, and three commits were rewritten with the tree unchanged. - **Restore sequence:** - the diff against `e28e832` was empty; - `git push --dry-run` with the lease reported a forced update of `e28e832` only; - `upstream/glmgampoi-r` was unchanged afterwards. Documentation only: no Julia, R or bun code changed, and no tests were run. ## Scope Leaves out any change to signing configuration. Nothing is pushed to `upstream/glmgampoi-r` here; that push is yours. No lockfile change. ## Contribution checks - [x] I followed [CONTRIBUTING.md](../CONTRIBUTING.md). - [ ] This branch starts at current upstream main and contains no merges from fork main. *(n/a: fork-only note, branched from fork `main`)* - [ ] CI has run and passes. *(pending on b18c3b2)* - [x] Existing licensing, citation and acknowledgement text is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01HtyPfZ1MWBB94RawgzQgz7 --------- Co-authored-by: Claude <noreply@anthropic.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
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.
Change and reason
Prepares the next push to upstream JoshuaJewell/MetaManifold-WebUI#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.wip/pr62-thetacommit, cherry-picked unchanged. It covers both of Joshua's code requests from Oct 19 (theta is1 / overdispersions; the Poisson boundary is read from it).glmGamPoi::test_deparses 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 asrun-Avsrun-BorgrpA/run_1vsgrpB/run_2, and label text that reads as R code was executed. The commit adds a regression test that covers both dispersions.overdispersionsis the per-taxon estimate before shrinkage. The coefficients and the F-test use the cross-taxon trend, and onlylfc_sereadsoverdispersions.dispersionwith 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:
When Joshua asks for a rebase (upstream
mainis nowce2c645), 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-Rtest_de,test_de_qandpredict.glmGamPoiwere 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:clr_welchtests that need zCompositions.CI=truemakes a missing glmGamPoi fail rather than skip, so every glmGamPoi test ran.Label experiments (the
fit_nbR block, run directly):run-B,grpB/run_2and2024-02failed every taxon, and a label of[file.create(...)]created the file.A/B, the file is not created, andglm_nbis unchanged.Null calibration (share of p < 0.05, binomial SE about 0.003):
Frontend:
bun test src39 pass; typecheck clean; build succeeds.Not run: the integration and server suites, the DADA2 taxonomy and phylogeny paths, and the
clr_welchtests (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.
Draft replacement description for upstream PR 62
Draft summary comment for upstream issue 37
Generated by Claude Code