Repository navigation
Conversation
PreprocessingConfig resolved its canonical and legacy fields once in __post_init__ and to_preprocessor_kwargs() only read that cache, so values changed through set_params() (including the preprocessing_config__* keys a GridSearchCV sets) or by attribute assignment never reached the preprocessor while get_params() reported them. to_preprocessor_kwargs() now validates and resolves the fields again whenever they changed since the last resolution. Fixes #489
This branch has not been deployed
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.
Fixes #489
Problem
Changing a
PreprocessingConfigafter construction had no effect on the preprocessor for most fields. Two kinds of change were affected:set_params(), including thepreprocessing_config__*keys that scikit-learn searches set on the estimator.get_params()reported the new values, but fitting used the old ones. AGridSearchCVoverpreprocessing_config__numerical_methodtherefore trained every candidate with the base preprocessing: candidates scored identically, andbest_estimator_disagreed withbest_params_.Root cause
PreprocessingConfig.__post_init__resolves canonical and legacy fields once into private_resolved_*attributes, andto_preprocessor_kwargs()reads only that cache.BaseEstimator.set_params()and attribute assignment never re-run__post_init__.Fix
__post_init__now records the field values the cache was resolved from.to_preprocessor_kwargs()re-runs validation and resolution whenever those values have changed. Every consumer goes throughto_preprocessor_kwargs(), so the fix lives in one place:SklearnBase.__init__/set_params_FitMixin._build_modelSklearnBaseLSS.build_modelbuild_preprocessorInvalid values set after construction are now rejected with the same validation errors the constructor raises, instead of being ignored. Configs that are not changed after construction behave exactly as before.
Tests
tests/configs/test_config_contracts.py:set_paramsand attribute assignment are reflected into_preprocessor_kwargs();InvalidParamErrorwhen it is used.tests/models/test_estimator_configuration.py:set_params(preprocessing_config__numerical_method=..., preprocessing_config__output_dim=...)reaches_preprocessor_kwargsand the fitted feature metadata;preprocessing_config.output_dim = 3assignment changes the fitted width.All 5 new tests fail on
mainand pass with this change.Docs
to_preprocessor_kwargs()documents that fields changed after construction are validated and resolved again. No user-facing page needed a change: the documentedGridSearchCVexample now works as described.Checks
just test: 3241 passed, 46 skipped, 28 xfailed; coverage 91.31%just docs: build succeeded (warnings treated as errors)just check: all hooks passed (ruff, prettier, pyright)