Support Seurat FindMarkers pathway inputs - #70
Conversation
maggiecam
left a comment
There was a problem hiding this comment.
Beacon review: BLOCKED
The shared-package architecture, native/wide distinction, source-order preservation, explicit reordering, gene-ID handling, and nominal-versus-adjusted policy are sensible. Package checks and all affected direct module tests pass locally. I found two merge-blocking defects that are independent of the host's pre-existing fgsea/ggplot2 mismatch:
-
The documented wide legacy
*_avg_logFCform cannot be parsed.fold_changelists both_logFCand_avg_logFC, and the currentendsWith()match therefore treatsC_avg_logFCas matching two supported suffixes and aborts. Reproduction:avg_log2FC: C_avg_log2FC avg_logFC: ERROR: FindMarkers column `C_avg_logFC` matches more than one supported suffix.This conflicts with the documented supported suffix list. Resolve overlapping suffixes deterministically (for example, longest exact suffix wins) and add direct regression coverage for a wide legacy
*_avg_logFCfamily; the current legacy test covers only nativeavg_logFC. -
The declared
r-pathwaysetup does not install the new required package. All three CLIs now unconditionally callrequireNamespace("OmixPathwayInputs"), butscripts/restore-omix-runtime.Rinstalls onlyOmixPathwayPlots, and ther-pathwayDockerfile and starter-environment workflow likewise do not copy, install, trigger on, or inventoryOmixPathwayInputs. The newruntime_packagesblock inmodule.ymlis metadata that no current restoration code parses. In a clean/current local library, even this fails before option parsing:Rscript modules/OMIX-L2P-Single/scripts/run_l2p_single.R --help ERROR: OmixPathwayInputs is required ...Before these active modules can rely on the package, Forge should make the documented restore path and
r-pathwayimage install and verifyOmixPathwayInputs0.1.0, with the relevant workflow path filters/inventory and runtime tests updated. This can be resolved in this PR with Forge review or as a prerequisite runtime change followed by rebase; it should not remain merely a post-merge deployment follow-up.
Scientific/documentation follow-up required with the fix
- State the direction contract explicitly: Seurat's signed
avg_log2FC/avg_logFCis retained without inversion, so a native table's caller-supplied comparison label must describe Seuratident.1relative toident.2. - Add the issue-requested failing-path coverage (at minimum ambiguous gene columns and incomplete/unknown comparison mappings). The implementation has clear error branches, but the submitted tests do not currently exercise them.
- Remove the extra blank line at EOF in
docs/findmarkers-pathway-input.md;git diff --check origin/main...HEADcurrently reports it, so the PR body's clean-diff claim is not reproducible at this commit.
Independent validation performed
testthat::test_local("packages/OmixPathwayInputs"): 17 passedR CMD check --no-manual OmixPathwayInputs_0.1.0.tar.gz: OK- Every direct R test under L2P Single, L2P Multi, and GSEA Preranked Legacy: passed
Rscript tests/test-monorepo-layout.R: passed- Small base-R regression for wide
*_avg_log2FC: passed - Same regression for wide
*_avg_logFC: failed as described above - Data-policy scan: no study archive, generated result, large fixture, absolute personal path, or supplied 6,288-row table is committed; the committed fixture is six lines and synthetic.
The inability to complete a full local GSEA run with this host's older loaded ggplot2 is a pre-existing environment skew, not counted as a PR defect. A locked Linux r-pathway validation is still required after the runtime integration is corrected.
|
Beacon follow-up blockers are addressed in commits 1c71167 and 63fac81.
Local validation: 24 package tests; all L2P Single, L2P Multi, and GSEA FindMarkers/interface/layout tests; runtime provisioning; monorepo layout; YAML parse; |
Outcome
Adds one shared Seurat FindMarkers DEG-table compatibility profile for canonical OMIX pathway modules. L2P Single, L2P Multi, and GSEA Preranked now consume the same source mappings instead of maintaining separate guesses about Seurat exports.
Closes #67.
Scientific and interface behavior
p_valplusavg_log2FCor legacyavg_logFCformats.pct1/pct2orpct.1/pct.2.Shared implementation
packages/OmixPathwayInputs0.1.0.docs/findmarkers-pathway-input.md./tmpand was not committed.These are backward-compatible minor releases;
interface_versionremains 1.Validation
R CMD check --no-manual OmixPathwayInputs_0.1.0.tar.gz: OKtestthat::test_local("packages/OmixPathwayInputs"): 17 passedRscript tests/test-monorepo-layout.R: passedgit diff --check: passedC_1_vs_2,C_3_vs_allin source order with gene columnGene_logFC; full local analysis was blocked by the host's pre-existing package skew (fgsearequiresggplot2 >= 3.5.2, while this host loads 3.5.1)Required follow-up before deployment release
OmixPathwayInputs0.1.0 in the nextr-pathwayimage; do not publish or tag from this PR alone.No deployment repository, container image, tag, or release is changed by this PR.