Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sktime clean clone method #1454

Closed
wants to merge 3 commits into from
Closed

sktime clean clone method #1454

wants to merge 3 commits into from

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Sep 25, 2021

Update: This actually does not seem to have been a problem with #1449. We can keep using sklearn clone.
Will close once it is confirmed that we can fix #1449 without.


This PR adds a clone method to BaseObject, which extends sklearn clone to deal with dynamic tags properly.

This is one potential way to address the issue in #1449, in which behaviour controlling tags inside grid/random search estimators are not cloned properly, and lead to unexpected error messages when multivariate forecasters are cloned.

As a proof-of-concept, the sklearn clone-s in the tuning module are also replaced by the new BaseObject.clone.

An added advantage is our new ability to modify the behaviour of clone in the BaseObject, in a modular fashion.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Sep 25, 2021

(happy to add tests if we decide to go with this solution)

@fkiraly
Copy link
Collaborator Author

fkiraly commented Sep 27, 2021

we don't need this, the problem this was trying to solve isn't actually one (cloning with dynamic tags works fine)

@fkiraly fkiraly closed this Sep 27, 2021
@fkiraly fkiraly deleted the tune_clone_fix branch March 21, 2022 23:05
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.

None yet

1 participant