fix: SklearnModelInfo.target_values returns both classes for binary classifiers - #231
Open
agu2347 wants to merge 1 commit into
Open
fix: SklearnModelInfo.target_values returns both classes for binary classifiers#231agu2347 wants to merge 1 commit into
agu2347 wants to merge 1 commit into
Conversation
…lassifiers `SklearnModelInfo.target_values` returned only the single "positive" class for binary classifiers (e.g. `["malignant"]`), but `JSONFiles.write_model_properties_json` requires either 0, 2, or >2 target values, raising "Please provide all possible values for the target variable, including a no-event value." for exactly 1. Since `_register_open_source_model` passes `info.target_values` straight into that function, registering any binary scikit-learn classifier via `register_model()` crashed with a ValueError. Return both classes (in `classes_` order, i.e. [non-event, event], matching the ordering already used for `output_column_names`/`predict_proba` columns) so binary classifiers register successfully. Fixes sassoftware#200
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.
Problem
Registering a binary scikit-learn classifier via
register_model()raises:Root cause
SklearnModelInfo.target_values(insrc/sasctl/utils/model_info.py) returns only the single "positive" class for binary classifiers:_register_open_source_model(intasks.py) passes this straight through toJSONFiles.write_model_properties_json(target_values=info.target_values), which explicitly requires either 0, 2, or >2 target values — and raises aValueErrorfor exactly 1:So any binary classifier hits this branch and fails.
info.target_valuesis also passed toImportModel().import_model(target_values=info.target_values, ...), whose docstring says "the order of the target values should reflect the order of the related probabilities in the model" — i.e. it expects both classes too.Closes #200
Fix
Return both classes for binary classifiers, in
classes_order ([non-event, event]), matching the ordering already used foroutput_column_names(e.g.["P_benign", "P_malignant"]) andpredict_probacolumns.Testing
test_sklearn_binary_classifierassertion, which previously pinned the single-value (buggy) behavior, to expect both classes.test_sklearn_binary_classifier_target_values_registration, which reproduces the exact reported crash by callingwrite_model_properties_jsonwithinfo.target_valuesfor a fitted binary classifier and asserting it no longer raises.model_info.pyonly, tests unchanged):AssertionError: assert ['malignant'] == ['benign', 'malignant']/assert 1 == 2.tests/unit/test_model_info_sklearn.pypass with the fix applied.black --checkpasses on both changed files.