Refuse the disabled Bayes options instead of ignoring them - #37
Conversation
mle accepted -p/--PPI-prior, -w/--PPI-weighting and -e/--negative-control and then never read them again. All three are only consumed inside mlemageck_bayes, which mlemageck does not import, so a run passing them exited 0 and wrote a complete gene_summary.txt produced without them -- indistinguishable from a run that had applied them. The practical consequence is a false comparison rather than a failed run: a user running "PPI on" against "PPI off" gets two result sets and concludes they compared two models. Confirmed on the bundled count table, where the beta, z and wald-p-value columns are byte-identical across -p, no -p, and a plain repeat; only the permutation p-values move, and those move between the two identical runs as well, because the permutation is unseeded. Refuse all three together, exiting non-zero and writing nothing, matching what --bayes already did in the same subsystem. Reporting them together means fixing one does not just reveal the next. -e additionally names --norm-method control with --control-sgrna, since mle has a real control-sgRNA option and that is what a user reaching for -e usually wants. The options stay registered so the message can be specific, with their help text marking them disabled. No deprecation period: deprecation protects working behavior during migration, and there is none here to protect. Any script passing these flags is already producing results that do not honor them. Closes #36. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bebc40912
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logging.error('To normalize against negative-control sgRNAs, use --norm-method control together ' | ||
| 'with --control-sgrna instead of -e/--negative-control.') |
There was a problem hiding this comment.
--control-gene
When a user replaces -e A1CF as this error recommends, --control-sgrna expects a file containing sgRNA IDs, whereas the disabled option accepted gene names. Supplying the same gene identifiers therefore finds fewer than two controls and aborts in mageckcount_checkcontrolsgrna; the supported equivalent is --norm-method control --control-gene <gene-list-file>, which expands genes to their sgRNAs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — confirmed and fixed in 2b00294.
Verified both directions on the bundled count table:
--norm-method control --control-sgrna genes.txt
-> 0 out of 2 control sgRNAs are found in count table, exit 255
--norm-method control --control-gene genes.txt
-> 2 gene(s) used as negative controls, 20 sgRNAs used as negative controls, exit 0
So the hint would have moved a user from a silently ignored option to a hard failure, which is the exact class of problem this PR exists to remove.
The message now names --control-gene, says the genes go in a file (-e took a bare name on the command line), and warns off --control-sgrna explicitly so the mistake cannot be made from the other direction:
To normalize against negative-control genes, list them one per line in a file and pass
--norm-method controlwith--control-gene. Note--control-sgrnais not the equivalent: it expects sgRNA IDs, and gene names given to it match nothing.
The test asserted --control-sgrna and so was green through the bug; it now asserts --control-gene and carries the reason. Help text and CHANGELOG updated to match, and following the corrected advice literally now runs to completion.
The hint added in the previous commit named the wrong replacement. -e
took a gene name, but --control-sgrna matches sgRNA IDs, so following the
advice moved the user from an ignored option to a hard failure:
--norm-method control --control-sgrna genes.txt
-> 0 out of 2 control sgRNAs are found in count table, exit 255
--norm-method control --control-gene genes.txt
-> 2 gene(s) used as negative controls, 20 sgRNAs, exit 0
--control-gene is the option that expands genes to their sgRNAs, and both
are on the mle subparser as a mutually exclusive pair. Name it instead,
say that the genes go in a file (-e took a bare name on the command
line), and warn off --control-sgrna explicitly so the same mistake is not
made from the other direction.
Reported by Codex review on #37.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
mleaccepted-p/--PPI-prior,-w/--PPI-weightingand-e/--negative-control, then ignored all three. The run exited 0 and wrote a complete, normal-looking result set produced without them.All three are read only inside
mlemageck_bayes/mageck_bayes_PPI, whichmlemageck.pynever imports.--bayesitself already errored and exited 1 — but its modifiers did not, which is the inconsistency users actually hit.The failure this causes
Not a failed run, a false belief. In #26 a user compared "PPI on" against "PPI off", got two result sets, and reasoned about a discrepancy against another tool from the difference. They were comparing one model against itself.
Confirmed on the bundled count table across three runs —
-p, no-p, and a plain repeat:beta,zandwald-p-valueare byte-identical across all three (cut -f1,3,4,7→ md506292574c19931d0aa71a7ab9b54534f)p-value/fdrcolumns move, and they move between the two identical commands too, because the permutation is unseededThe change
Refuse all three together, exiting non-zero and writing nothing — matching
--bayesin the same subsystem. Reported together so fixing one flag does not just reveal the next:-egets that second line because it is the trap of the three:mlehas a real--control-sgrna, so a user passing-ereasonably believes they specified negative controls for normalization.The options stay registered rather than being deleted, so the error can be specific instead of argparse's generic
unrecognized arguments— the approach taken for--pairguide autoin #33. Their help text now marks them disabled.No deprecation period. Deprecation protects working behavior during migration; there is none here to protect, and a release spent warning is a release spent letting people draw the #26 conclusion.
Tests
Six new tests, written failing first — each previously got
returncode == 0and agene_summary.txton disk. They cover-p, the--PPI-priorlong form,-w,-e, the--control-sgrnapointer, and all three reported in one message. Each asserts no output file is written, not just the exit code._run_mlegained anextra_argsparameter (behavior-preserving). Suite: 43 → 49, all passing.Closes #36.