Skip to content

fix: always reduce the early stopping set to available features in train_final_model - #462

Open
jfrog64 wants to merge 3 commits into
daisybio:developmentfrom
jfrog64:fix-final-model-early-stopping-reduction
Open

fix: always reduce the early stopping set to available features in train_final_model#462
jfrog64 wants to merge 3 commits into
daisybio:developmentfrom
jfrog64:fix-final-model-early-stopping-reduction

Conversation

@jfrog64

@jfrog64 jfrog64 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

PR Checklist for all PRs

  • This comment contains a description of changes (with reason)
  • Referenced issue is linked — no existing issue, found while running final models with early stopping
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated — no user-facing API change, so nothing to update

Changes

Bug fixes

In train_final_model, the training set is reduced to the cell lines and drugs that actually have
features, and a matching reduction exists for the early stopping set. That second reduction, however,
sits inside if response_transformation::

if response_transformation:
    train_dataset.fit_transform(response_transformation)
    if early_stopping_dataset is not None:
        early_stopping_dataset.reduce_to(cell_line_ids=cell_lines_to_keep, drug_ids=drugs_to_keep)
        ...
        early_stopping_dataset.transform(response_transformation)

So whenever a final model is trained with early stopping but without a response transformation
(the default), the early stopping set is never reduced. The model then receives early stopping cell
lines / drugs that have no feature row, while the training set has already been filtered. Depending
on the model this surfaces as a KeyError on the missing identifier, or as a silently wrong early
stopping signal.

The reduction is not conditional on the transformation — it depends only on which features are
available — so this moves it out of the if block. The transformation call stays where it was.

Tests: tests/test_final_model_early_stopping.py drives train_final_model with a minimal early
stopping model whose features cover only half of the cell lines and with response_transformation=None.
It first asserts the precondition (the raw early stopping split really does contain cell lines without
features) so the test cannot go vacuous, then asserts that train() only ever sees cell lines that
have a feature row. Against the current development the test fails; with the fix it passes.

The full suite is green locally (219 passed).

New features

Maintenance


Follow-up commit: response_transformation annotation

The typeguard session flagged the regression test, because train_final_model was annotated
response_transformation: TransformerMixin — without | None. The test has to pass None, since the
bug only occurs on the path without a response transformation.

The annotation is simply wrong, and it is the only one of the twelve response_transformation
parameters in experiment.py that lacks | None:

  • the only internal caller, experiment.py:463, forwards run_suite's response_transformation: TransformerMixin | None = None
  • get_response_transformation is declared -> TransformerMixin | None and returns None for "None"
  • the body of train_final_model itself guards with if response_transformation:

mypy cannot catch this: sklearn ships no type stubs, so TransformerMixin is Any and passing None
is unremarkable. Only the runtime check in the typeguard session sees it. The second commit widens the
annotation to TransformerMixin | None and updates the docstring; no behaviour changes.

jfrog64 and others added 3 commits August 26, 2026 15:58
train_final_model() only reduced the early stopping dataset to the cell
lines and drugs that actually have features when a response
transformation was configured, because the reduce_to() call was nested
inside the `if response_transformation:` branch. Without a
transformation the early stopping set kept samples whose features are
missing, and training the final model aborted with

    AssertionError: 4 of 21 ids are not in the FeatureDataset.
    Missing ids: {'COLO 205', 'BT 271', 'OVCAR-5', 'DLD-1'}

raised by FeatureDataset.get_feature_matrix() as soon as the model
looked up the early stopping features. This affects every model with
early_stopping = True (DIPK, SimpleNeuralNetwork, MOLIR, SuperFELTR,
PharmaFormer, ...) when --final_model_on_full_data is used without
--response_transformation, i.e. the plain default setup: the whole
cross-validation completes and only the final model fails.

The reduction is now performed unconditionally, mirroring what
train_and_predict() already does, and the transformation branch only
transforms.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test drives train_final_model with a minimal early stopping model whose
features cover only half of the cell lines and no response transformation.
It asserts the precondition (the raw early stopping split does contain cell
lines without features) so it cannot go vacuous, and then that train() only
sees cell lines that have a feature row.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parameter was annotated as TransformerMixin, but None is a valid and in
fact the default value: run_suite declares it as TransformerMixin | None,
get_response_transformation returns None for "None", and the body guards with
`if response_transformation:`. It is the only one of the twelve
response_transformation parameters in experiment.py without | None.

mypy does not catch this because sklearn ships no type stubs, so
TransformerMixin is Any; the typeguard session does, as soon as a test calls
train_final_model without a transformation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 96.36364% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.12%. Comparing base (7d24cd6) to head (6bdf0d0).
⚠️ Report is 75 commits behind head on development.

Files with missing lines Patch % Lines
tests/test_final_model_early_stopping.py 96.00% 2 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #462      +/-   ##
===============================================
+ Coverage        80.34%   82.12%   +1.78%     
===============================================
  Files              101      121      +20     
  Lines             8171     9831    +1660     
===============================================
+ Hits              6565     8074    +1509     
- Misses            1606     1757     +151     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants