Fix 6 correctness bugs found in code review - #247
Merged
Conversation
A1. prime.py / mixmhc2pred.py: delete the redundant predict_dataframe
overrides. They duplicated BasePredictor.predict_dataframe but with a
narrower signature that dropped n_flanks/c_flanks, so
predict_dataframe(peps, n_flanks=[...]) raised TypeError even though both
predict() methods accept flanks. Inherit the base method instead (and drop
the now-unused COLUMNS import).
A2. cli/script.py: give a clear error when a new-model-only predictor (bigmhc,
calis, deeptap, eramer, netchop, pepsickle — predict() but no
predict_peptides) is used on the legacy prediction CLI, pointing to the
predict-table subcommand, instead of failing later with AttributeError.
A3. annotate.py: the predict-table `stability` token mapped to `value`, but
NetMHCstabpan reports half-life in `score` (parse_netmhcstabpan sets no
ic50), so the column was always all-NaN. Map it to `score`.
A4. annotate.py: a kind-agnostic token (score/percentile_rank/rank) matched
every kind, so on a multi-kind predictor (e.g. MHCflurry, which emits
affinity + presentation for the same peptide+allele) the (peptide, allele)
key was silently overwritten last-one-wins — `mhcflurry:col:score` returned
presentation, not affinity. Now raise an "ambiguous field" error directing
the caller to a kind-specific token.
A5. cli/args.py: register the NetCleave family (netcleave / netcleave-i /
netcleave-ii). They were exported and in __all__ but absent from the
registry, so unusable via predict-table (inconsistent with netchop/pepsickle).
A6. nettcr.py: make _suppress_native_stderr leak-safe — acquire both fds inside
the try and close-if-not-None in finally, so a failure at os.open (fd
exhaustion, exactly when it matters) can't leak the already-dup'd fd 2.
Tests: regression tests for each (flank signature inherited; legacy-CLI guard;
stability reads score; ambiguous multi-kind token raises while kind-specific
works; NetCleave registered). Verified A2/A4 end-to-end against calis/mhcflurry.
Version 3.27.0 -> 3.28.0.
Claude-Session: https://claude.ai/code/session_01LZahFhBSCiehXTESCYQ7wG
iskandr
added a commit
that referenced
this pull request
Jul 10, 2026
… cleanup (#248) Follow-up to the code-review correctness PR (#247). Structural + cosmetic: net -129 lines in the wrapper files with no behavior change (except the BigMHC fixes below). Consolidation (new mhctools/wrapper_base.py): - NewModelPredictorMixin holds the members that were copy-pasted across the standalone wrappers: predict_dataframe, supported_kinds, __repr__, and peptide normalization (single-string + strip/upper). - AlleleFreePredictor adds the allele-independent kind_support shape. - Calis / DeepTAP / ERAMER now subclass AlleleFreePredictor; BigMHC uses NewModelPredictorMixin. Each keeps only its constructor, _default_pred_kind, tool-specific validation (bounds/messages differ), and scoring. BigMHC consistency fixes (were flagged in review): - Build predictions from the *normalized* peptide, not the raw input, so "siinfekl " is no longer scored as SIINFEKL but returned verbatim. - Rename _pred_kind -> _default_pred_kind to match every other wrapper (generic consumers can now call it uniformly). Minor code: - eramer: early-return on empty input (was loading the whole PWM first); drop the unreachable score-is-None branch; delete the dead _predictor_name. - calis/deeptap: delete the dead _predictor_name. - pred: add reduce_op(kind, field) and use it in PeptideResult.best_by and AnnotationSpec.direction_op (the max/min mapping was written twice). - mixmhc2pred: resolve the %Rank_/Score_ column positions once, not per row. Docs / LLMism cleanup: - Trim the copy-pasted "Note on interpretation" caveats to one sentence each (calis/prime/eramer/deeptap); drop PRIME's benchmark editorializing and the marketing/hedging phrasing; remove **bold** from reST docstrings. - Fix stale docstrings: calis position_weights ("leading slice"), eramer's class docstring (add $ERAMER_PWM to the resolution order). - Trim the bloated best_direction docstring, filler class docstrings, an over-defensive annotate comment, and the verbose nettcr suppression docstring. Tests: new tests/test_wrapper_base.py covers the shared base and asserts the wrappers inherit it (and BigMHC standardizes on _default_pred_kind). Full public subset: 496 passed. Version 3.28.0 -> 3.29.0. Claude-Session: https://claude.ai/code/session_01LZahFhBSCiehXTESCYQ7wG
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
1 participant
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.
First of two PRs from a full-package code review (4 parallel reviewers + verification). This one is the behavioral bug fixes; a follow-up handles the wrapper-consolidation refactor + LLMism/docstring cleanup.
Each fix has a regression test; A2 and A4 were also verified end-to-end.
PRIME/MixMHC2predpredict_dataframeoverrides dropn_flanks/c_flanks→TypeError, thoughpredict()accepts them. 100% redundant with the base.BasePredictor.predict_dataframe); drop the now-unusedCOLUMNSimport.bigmhc,calis,deeptap,eramer,netchop,pepsickle) crash withAttributeErroron the legacy--mhc-predictor X --sequenceCLI (they havepredict()but nopredict_peptides). The help text even showspepsickle.predict-table.predict-tablestabilitycolumn is always all-NaN — the token mapped tovalue, but NetMHCstabpan puts half-life inscore(parse_netmhcstabpansets no ic50).stability→score.score/ranktokens silently return the wrong kind on multi-kind predictors:mhcflurry:col:scoregave presentation, not affinity (last-wins withkind=None).NetCleave*exported + in__all__but not in the CLI registry → unusable viapredict-table(inconsistent withnetchop/pepsickle).netcleave/netcleave-i/netcleave-ii.nettcr._suppress_native_stderrleaks an fd ifos.open(devnull)fails (acquired beforetry).try, close-if-not-None infinally.Verified end-to-end:
mhcflurry:col:scorenow raisesAmbiguous 'score' field: predictor emits multiple kinds (pMHC_affinity, pMHC_presentation)…;--mhc-predictor calis --sequence …now raisesCalis does not support this command … Use \mhctools predict-table` instead.` Full public test subset: 492 passed.Version
3.27.0 → 3.28.0.https://claude.ai/code/session_01LZahFhBSCiehXTESCYQ7wG