[ENH] set_params to call reset, to comply with sklearn parameter interface assumptions#2835
[ENH] set_params to call reset, to comply with sklearn parameter interface assumptions#2835
set_params to call reset, to comply with sklearn parameter interface assumptions#2835Conversation
set_params to call resetset_params to call reset, to comply with sklearn parameter interface assumptions
|
Works, and seems to dredge up two genuine bugs with |
|
the bug was not with the multiplexers, but with the test (parameters not overwritten by defaults in the loop) |
|
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. |
Well, it wasn´t one, I made a stupid mistake. I was testing whether you could |
…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>
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.
This PR modifies
set_paramsto also execute a fullreset, 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 namesget_param_defaults- retrieves name/default dict of parameter defaultsWhy
set_paramsshould execute areset: @aiwalter pointed me to this specification document insklearnwhich makes a key interface assumption explicit, namely that the result ofset_paramsmust be the same as__init__(**self.get_params()).Since
sktimemay have some__init__content besidesselfwrites (in deviation fromsklearndue to varia discussed in #2573), we can ensure consistency here by callingresetfromset_params.This relates to discussion in, and also partially addresses #2573 .
As a secondary effect of this, the
test_set_paramstest was failing in instances where parameter related logic was present in__init__, e.g., cloning estimators tocomponent_name_, due to testing logic insklearn'scheck_set_paramswhich was called fromtest_set_params. The reason for that is thatsklearn's test would overwrite parameters withnp.infetc to mimic a change, which is fine insklearnsince it strictly has no logic in__init__. However,sktimeestimators may have logic, such as dynamic tag setting, which therefore would break theset_paramsnow identical with__init__.Therefore, the
test_set_paramstest was changed from overwriting withnp.infetc, to overwriting with values from other test cases as available throughget_test_paramsandget_param_defaults(needed for full set), which ensures validity.