[BUG] fix unreported set_params bug in ClassifierPipeline and RegressorPipeline
#3857
Conversation
This file contains 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
fkiraly
added
module:classification
aiwalter
previously approved these changes
Nov 30, 2022
|
Hmmm, any thoughts on the error messages? Seem to be specific to this PR, but only crop up in certain python versions I am guessing? |
The fact that it´s in different versions is coming from estimators being split across version/OS combinations to minimize runtime of tests |
fkiraly
added a commit
that referenced
this pull request
Dec 22, 2022
This adds an equality dunder to `BaseObject` descendants. Two `BaseObject` descendants are considered equal if they define the same estimator blueprint, i.e., nested `get_params` result in value-identical dictionaries (with nested call to `__eq__` should those dictionaries contain `BaseObject` descendants). Includes a test for expected properties of the dunder. This is prompted by issues of equality testing in tests such as in the failure relevant for #3857, where equality currently means object identity rather than parameter identity. FYI @RNKuhns, this could/should be an `skbase` feature, so would appreciate opinion on whether it interacts with any of the other tests or properties.
fkiraly
added a commit
that referenced
this pull request
Feb 13, 2023
…3631) This PR isolates `numba` throughout `sktime`. Note: does *not* turn `numba` into a soft dependency. This is required for all approaches to making `sktime` python 3.11 compatible as long as `numba` is not python 3.11 compatible, because that requires being able to run the framework components of `sktime` without `numba` imports. See also #3963. The general recipe to isolate `numba`: * move `numba` imports into functions and classes, where possible * where loose functions are decorated with `jit` and `njit`, move them to a private module `_orig_modulename_numba`, and move imports of that into functions or classes * add the package dependency tag to `numba` dependent estimators to isolate them * use `pytest.skipif` to conditionally skip `numba` dependent non-suite tests if `numba` is not installed Makes the following changes: * changes the `test_module_softdeps` test to ignore private modules (starting with `_`), i.e., to not require a user warning. This is ok since users are not expected to import private modules. Depends on #3857
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.
set_paramsbug inClassifierPipelineandRegressorPipelinewas broken, it would not correctly update parameters.The failure to detect this is an instance of the known problem #3429
The bug has been fixed, and is now covered by appropriate tests (addition of a second parameter set).
The fix is as follow:
set_paramswhich accidentally had one too many layer of nesting in the param dict indexing, e.g.,classifier__etc. This would materialize only for doubly nested estimators._HetereogenousMetaEstimator.Depends on #3858 as the
DummyClassifieris used as one of two param sets.