Refuse the disabled Bayes options instead of ignoring them

CHANGELOG.md

@@ -43,6 +43,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `mle` no longer accepts `-p`/`--PPI-prior`, `-w`/`--PPI-weighting` or
+  `-e`/`--negative-control` and then ignores them. All three are read only
+  inside the experimental Bayes module, which is disabled, so a run passing them
+  exited 0 and wrote a complete result set produced without them — a "PPI on"
+  and a "PPI off" run gave the same model, differing only in unseeded
+  permutation noise. They are now refused together, exiting non-zero and writing
+  nothing, matching what `--bayes` already did. `-e` additionally points at
+  `--norm-method control` with `--control-gene`, which is what a user reaching
+  for it usually wants — and warns off `--control-sgrna`, which takes sgRNA IDs
+  rather than the gene names `-e` accepted. See issue #36.
 - `--pairguide firstpair|secondpair` and `--umi firstpair|secondpair` no longer
   accept a missing extraction window. The window was checked inside the per-read
   loop, which logged an error for every read — one line per read on a real FASTQ

mageck2/argsParser.py

@@ -208,10 +208,10 @@ def arg_mle(subparser):
   emgroup.add_argument('--update-efficiency',action='store_true',help='Iteratively update sgRNA efficiency during EM iteration.')
   ## Bayes option
   bayesgroup=subm_mle.add_argument_group(title='Optional arguments for the Bayes estimation of gene essentiality (experimental)',description='')
-  bayesgroup.add_argument("--bayes",action='store_true',help="Use the experimental Bayes module to estimate gene essentiality")
-  bayesgroup.add_argument("-p","--PPI-prior",action='store_true',help="Specify whether you want to incorporate PPI as prior")
-  bayesgroup.add_argument("-w","--PPI-weighting",type=float,help="The weighting used to calculate PPI prior. If not provided, iterations will be used.",default=None)
-  bayesgroup.add_argument("-e","--negative-control",help="The gene name of negative controls. The corresponding sgRNA will be viewed independently.",default=None)
+  bayesgroup.add_argument("--bayes",action='store_true',help="Use the experimental Bayes module to estimate gene essentiality. Currently disabled: passing this option is an error.")
+  bayesgroup.add_argument("-p","--PPI-prior",action='store_true',help="Specify whether you want to incorporate PPI as prior. Part of the disabled --bayes module; currently an error.")
+  bayesgroup.add_argument("-w","--PPI-weighting",type=float,help="The weighting used to calculate PPI prior. If not provided, iterations will be used. Part of the disabled --bayes module; currently an error.",default=None)
+  bayesgroup.add_argument("-e","--negative-control",help="The gene name of negative controls. The corresponding sgRNA will be viewed independently. Part of the disabled --bayes module; currently an error -- for control-based normalization put the gene names in a file and use --norm-method control with --control-gene.",default=None)
 
 def arg_run(subparser):
   """

mageck2/mlemageck.py

@@ -39,9 +39,28 @@ def mageckmle_main(pvargs=None,parsedargs=None,returndict=False):
   else:
     args=mageckmle_parseargs(pvargs)
   args=mageckmle_postargs(args)
-  # Bayes module
-  if hasattr(args,'bayes') and args.bayes:
-    logging.error('The experimental mle --bayes module is currently disabled and not supported in this release.')
+  # Bayes module. These options are only ever read inside mlemageck_bayes, which
+  # this module does not import, so accepting any of them would run a plain MLE
+  # and write results that look as though the option had been applied. Refuse
+  # them together, so fixing one does not just reveal the next. See issue #36.
+  disabled=[]
+  if getattr(args,'bayes',False):
+    disabled.append('--bayes')
+  if getattr(args,'PPI_prior',False):
+    disabled.append('-p/--PPI-prior')
+  if getattr(args,'PPI_weighting',None) is not None:
+    disabled.append('-w/--PPI-weighting')
+  if getattr(args,'negative_control',None) is not None:
+    disabled.append('-e/--negative-control')
+  if len(disabled)>0:
+    (noun,obj,subj)=('option','it','it was') if len(disabled)==1 else ('options','them','they were')
+    logging.error('The experimental mle --bayes module is currently disabled and not supported in '
+        'this release, so the following '+noun+' cannot be honored: '+', '.join(disabled)+'. Rerun '
+        'without '+obj+'; no other results change, because '+subj+' never applied.')
+    if '-e/--negative-control' in disabled:
+      logging.error('To normalize against negative-control genes, list them one per line in a file '
+          'and pass --norm-method control with --control-gene. Note --control-sgrna is not the '
+          'equivalent: it expects sgRNA IDs, and gene names given to it match nothing.')
     sys.exit(1)
   # from mleclassdef import *
   # from mledesignmat import *

tests/test_smoke.py

@@ -11,6 +11,8 @@
 import subprocess
 from pathlib import Path
 
+import pytest
+
 DATA = Path(__file__).parent / "data" / "count_table.txt"
 GMT = Path(__file__).parent / "data" / "pathways.gmt"
 
@@ -410,14 +412,15 @@ def _write_designmat(tmp_path, name, rows, header="Samples\tbaseline\tHL60\tKBM7
     return p
 
 
-def _run_mle(tmp_path, designmat, prefix):
+def _run_mle(tmp_path, designmat, prefix, extra_args=()):
     return subprocess.run(
         [
             "mageck2", "mle",
             "-k", str(DATA),
             "-d", str(designmat),
             "-n", prefix,
             "--permutation-round", "1",
+            *extra_args,
         ],
         cwd=tmp_path,
         capture_output=True,
@@ -506,6 +509,69 @@ def test_mle_accepts_valid_designmat_with_pooled_baselines(tmp_path):
     assert "HL60|beta" in header and "KBM7|beta" in header
 
 
+# The experimental Bayes module is disabled (--bayes exits 1), and these three
+# options only have meaning inside it. They used to be accepted and ignored, so
+# a run "with PPI" and a run "without" produced the same model -- see issue #36.
+_DISABLED_BAYES_ARGS = [
+    ("-p", ("-p",), "--PPI-prior"),
+    ("--PPI-prior", ("--PPI-prior",), "--PPI-prior"),
+    ("-w", ("-w", "0.5"), "--PPI-weighting"),
+    ("-e", ("-e", "A1CF"), "--negative-control"),
+]
+
+
+@pytest.mark.parametrize(
+    "label,argv,named", _DISABLED_BAYES_ARGS, ids=[a[0] for a in _DISABLED_BAYES_ARGS]
+)
+def test_mle_rejects_disabled_bayes_options(tmp_path, label, argv, named):
+    """A disabled option must not produce output that looks like it was honored.
+
+    Each of these reached argparse, was never read again, and left a complete
+    gene_summary.txt behind at exit 0 -- indistinguishable from a run that had
+    applied it.
+    """
+    designmat = _write_designmat(tmp_path, "good.txt", _GOOD_ROWS)
+
+    result = _run_mle(tmp_path, designmat, "disabled", extra_args=argv)
+
+    assert result.returncode != 0, label + " must not be silently ignored"
+    combined = result.stdout + result.stderr
+    assert named in combined, "the error must name the option that was refused"
+    assert "disabled" in combined
+    assert not (tmp_path / "disabled.gene_summary.txt").exists(), (
+        "refusing the option must also mean writing no results"
+    )
+
+
+def test_mle_negative_control_points_at_the_supported_option(tmp_path):
+    """-e is the trap of the three: mle has real control-based normalization too.
+
+    It must name --control-gene, not --control-sgrna. -e took a *gene* name,
+    and --control-sgrna matches sgRNA IDs -- feeding it gene names finds zero
+    controls and exits 255, so the wrong pointer just relocates the dead end.
+    """
+    designmat = _write_designmat(tmp_path, "good.txt", _GOOD_ROWS)
+
+    result = _run_mle(tmp_path, designmat, "negctl", extra_args=("-e", "A1CF"))
+
+    combined = result.stdout + result.stderr
+    assert "--control-gene" in combined and "--norm-method control" in combined
+
+
+def test_mle_reports_every_disabled_option_at_once(tmp_path):
+    """Fixing one flag only to be stopped by the next is a poor way to learn."""
+    designmat = _write_designmat(tmp_path, "good.txt", _GOOD_ROWS)
+
+    result = _run_mle(
+        tmp_path, designmat, "allthree", extra_args=("-p", "-w", "0.5", "-e", "A1CF")
+    )
+
+    assert result.returncode != 0
+    combined = result.stdout + result.stderr
+    for named in ("--PPI-prior", "--PPI-weighting", "--negative-control"):
+        assert named in combined, named + " missing from the combined error"
+
+
 def test_documented_designmat_example_is_valid():
     """The example in `mageck2 mle --help` must satisfy the design-matrix rules.