fix: thread PCA n_pcs and external covariates into GWAS assoc step - #247
Merged
Merged
Conversation
The assoc branch of PipelineRunner._run_single_step threaded only `build` and `maf_lambdas` into the config, silently dropping three user-supplied GWAS args: - `--pca N` (n_pcs): PCAConfig was built without n_pcs, so PCA always ran the default 10 PCs regardless of the requested count. - `--covars` / `--covar-names`: never reached AssocConfig.covariates, so external covariates were ignored and GWAS fell back to PCA covariates. Now passes n_pcs into PCAConfig and builds a CovariateConfig from the covar path/names when given. run_pca/run_gwas are normalized to bool to match the dataclass field types. Adds CovariateConfig to the runner's config-class map. Regression tests (tests/unit/test_cli/test_runner_regression.py): - n_pcs and external covariates reach AssocConfig (hermetic, capture the config passed to run_association) - build threading is preserved - end-to-end: --pca 3 produces a 3-PC eigenvec via real PLINK2 (was 10)
Extends the old-vs-new parity suite to cover the two args the runner used to drop (fixed in the previous commit): - test_old_vs_new_pca_ncount_parity: --pca 20 must produce a 20-PC eigenvec in BOTH CLIs (new previously always wrote 10). - test_old_vs_new_gwas_external_covars_parity: with --pca --gwas --covars --covar-names, both CLIs discard the PCA eigenvectors and use the external covariate file, so decision B does not apply and per-variant p-values must agree tightly. Before the fix the new CLI ignored --covars and fell back to PCA covariates (40500 p-mismatches, lambda 0.978 vs 1.007); now they match. Both tests verified to fail against the pre-fix runner and pass after. They skip cleanly without .venv-stable/plink2.
- REFACTOR_HARDENING.md: add round-3 section (assoc-branch arg threading fix + parity harness extension); mark remaining-work item 8 done. - TESTING.md: bump suite count 391 -> 397; add the two new parity rows.
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
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.
Summary
PipelineRunner._run_single_step(assoc branch), which threaded onlybuildandmaf_lambdasintoAssocConfigand silently dropped three user-supplied GWAS args:--pca N(requested PC count):PCAConfigwas built withoutn_pcs, so PCA always ran the default 10 PCs regardless of--pca N.--covars/--covar-names: never reachedAssocConfig.covariates, so external covariates were silently ignored and GWAS fell back to PCA eigenvectors as covariates.n_pcsintoPCAConfigand builds aCovariateConfigfrom the covar path/names when given; normalizesrun_pca/run_gwasto bool. AddsCovariateConfigto the runner's config-class map.REFACTOR_HARDENING.md(item 8 done) andTESTING.md.This directly affects real-cohort GWAS: before the fix,
--pca Nand external--covarswere ignored by the new CLI.What changed
genotools/cli/runner.py— assoc branch threadsn_pcs+ covariates;CovariateConfigadded to config-class map.tests/unit/test_cli/test_runner_regression.py— newTestAssocStepThreadsPcaAndCovariateArgs(hermetic capture of the config passed torun_association: n_pcs + external covars reachAssocConfig,buildpreserved) andTestAssocStepPcaProducesRequestedPcs(end-to-end--pca 3 → 3-PC eigenvecvia real PLINK2, was 10).tests/regression/test_parity.py—test_old_vs_new_pca_ncount_parity(--pca 20→ 20-PC eigenvec in both CLIs) andtest_old_vs_new_gwas_external_covars_parity(external covars used identically → p-values agree; decision B does not apply because the PCA eigenvectors are discarded for external covars).Test plan
.venv-stablepresent..venv-stable/plink2.refactor/main→main).Notes
refactor/main, notmain.