Skip to content

&hazrulakmal [BUG] fix clone for nested sklearn estimators#195

Merged
fkiraly merged 5 commits into
mainfrom
fix-clone
Jun 16, 2023
Merged

&hazrulakmal [BUG] fix clone for nested sklearn estimators#195
fkiraly merged 5 commits into
mainfrom
fix-clone

Conversation

@fkiraly
Copy link
Copy Markdown
Contributor

@fkiraly fkiraly commented Jun 16, 2023

The clone method of BaseObject would not properly clone sklearn estimator that are components (which do not have their own clone). See sktime/sktime#4704 for a downstream effect.

For now, this is fixed by replacing the faulty skbase clone with a copy of the sklearn clone.

Also adds a test case similar to the failure reported originally here: sktime/sktime#4704

Comment thread skbase/base/_base.py Fixed
@fkiraly fkiraly changed the title [BUG] fix clone for nested sklearn estimators &hazrulakmal [BUG] fix clone for nested sklearn estimators Jun 16, 2023
Comment thread skbase/base/_base.py
estimator_type = type(estimator)
# XXX: not handling dictionaries
if estimator_type in (list, tuple, set, frozenset):
return estimator_type([self._clone(e, safe=safe) for e in estimator])

Check failure

Code scanning / CodeQL

Wrong number of arguments in a class instantiation

Call to [BaseObject.__init__](1) with too many arguments; should be no more than 0.
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 16, 2023

Codecov Report

Merging #195 (3c6a865) into main (669d9a9) will decrease coverage by 0.34%.
The diff coverage is 82.60%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
- Coverage   82.75%   82.41%   -0.34%     
==========================================
  Files          43       43              
  Lines        2806     2832      +26     
==========================================
+ Hits         2322     2334      +12     
- Misses        484      498      +14     
Impacted Files Coverage Δ
skbase/base/_base.py 78.10% <82.60%> (-1.96%) ⬇️

... and 1 file with indirect coverage changes

@fkiraly fkiraly merged commit 1e20102 into main Jun 16, 2023
fkiraly added a commit to sktime/sktime that referenced this pull request Jun 20, 2023
… object (#4707)

This adds a test case for the failure of `set_params` with nested
`sklearn` objects, see #4704

The fix is in `skbase` and is also tested there:
sktime/skbase#195

This PR adds a test in `sktime` with the `sktime` objects for which the
failure was reported in #4704.
CTFallon pushed a commit to CTFallon/sktime that referenced this pull request Jun 21, 2023
… object (sktime#4707)

This adds a test case for the failure of `set_params` with nested
`sklearn` objects, see sktime#4704

The fix is in `skbase` and is also tested there:
sktime/skbase#195

This PR adds a test in `sktime` with the `sktime` objects for which the
failure was reported in sktime#4704.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants