Skip to content

[ENH] set_params to call reset, to comply with sklearn parameter interface assumptions#2835

Merged
lmmentel merged 8 commits intomainfrom
set_params_reset
Jun 28, 2022
Merged

[ENH] set_params to call reset, to comply with sklearn parameter interface assumptions#2835
lmmentel merged 8 commits intomainfrom
set_params_reset

Conversation

@fkiraly
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly commented Jun 20, 2022

This PR modifies set_params to also execute a full reset, which calls any __init__ code such as dynamic tag setting.

Also adds parameter inspection related class methods to BaseObject:

  • get_param_names - retrieves list of parameter names
  • get_param_defaults - retrieves name/default dict of parameter defaults

Why set_params should execute a reset: @aiwalter pointed me to this specification document in sklearn which makes a key interface assumption explicit, namely that the result of set_params must be the same as __init__(**self.get_params()).

Since sktime may have some __init__ content besides self writes (in deviation from sklearn due to varia discussed in #2573), we can ensure consistency here by calling reset from set_params.
This relates to discussion in, and also partially addresses #2573 .

As a secondary effect of this, the test_set_params test was failing in instances where parameter related logic was present in __init__, e.g., cloning estimators to component_name_, due to testing logic in sklearn's check_set_params which was called from test_set_params. The reason for that is that sklearn's test would overwrite parameters with np.inf etc to mimic a change, which is fine in sklearn since it strictly has no logic in __init__. However, sktime estimators may have logic, such as dynamic tag setting, which therefore would break the set_params now identical with __init__.

Therefore, the test_set_params test was changed from overwriting with np.inf etc, to overwriting with values from other test cases as available through get_test_params and get_param_defaults (needed for full set), which ensures validity.

@fkiraly fkiraly added the implementing framework Implementing or improving framework for learning tasks, e.g., base class functionality label Jun 20, 2022
@fkiraly fkiraly changed the title [ENH] set_params to call reset [ENH] set_params to call reset, to comply with sklearn parameter interface assumptions Jun 20, 2022
@fkiraly fkiraly added the module:tests test framework functionality - only framework, excl specific tests label Jun 20, 2022
@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jun 20, 2022

Works, and seems to dredge up two genuine bugs with MultiplexForecaster and MultiplexTransformer. Will look at it later, perhaps you might have an idea what is going on here, @miraep8?

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jun 24, 2022

the bug was not with the multiplexers, but with the test (parameters not overwritten by defaults in the loop)

@miraep8
Copy link
Copy Markdown
Collaborator

miraep8 commented Jun 25, 2022

Sorry I missed the original request to review a few days ago. Glad the bug was resolved. Threw in a few ideas/thoughts that came up while looking over the code.

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jun 25, 2022

Glad the bug was resolved

Well, it wasn´t one, I made a stupid mistake.

I was testing whether you could set_params with a second feature set, by getting param sets from get_test_params. In cases where the first parameter set had parameters that the second did not overwrite, this could lead in inconsistent parameter combinations. To obtain consistent parameter combinations, one needs to revert those parameters (from the first set that the second does not overwrite) to the defaults, which the test now does.

@lmmentel lmmentel self-requested a review June 28, 2022 17:00
@lmmentel lmmentel merged commit d77d1e8 into main Jun 28, 2022
@lmmentel lmmentel deleted the set_params_reset branch June 28, 2022 18:53
tobias-weiss-ai-xr pushed a commit to tobias-weiss-ai-xr/sktime that referenced this pull request Jul 5, 2022
…r interface assumptions (sktime#2835)

* set_params

* forgot return self

* translated sklearn set_params tests to sktime

* bugfix test

* remove unused import

* separate assertion from set_params

* param defaults and tests

Co-authored-by: Lukasz Mentel <lmmentel@users.noreply.github.com>
fkiraly added a commit that referenced this pull request Aug 12, 2023
In order to test parameter settings for estimators properly, each
important parameter should have been set to a non-default.
This PR introduces a test which checks that there are at least two test
parameter sets (if the estimator has at least one parameter), and
correctness of these sets.

Requires:
* #4279 for the general test for
`get_test_params` (no specific number assumed)
* #2835 as it uses
the parameter name interface
* #3428 to pass the
`no-softdeps` step.

Current estimators are not tested due to differential testing.

If diff testing is turned off, helps tracking down the estimators that
do not yet have two parameter sets, see #3429.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

implementing framework Implementing or improving framework for learning tasks, e.g., base class functionality module:tests test framework functionality - only framework, excl specific tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants