[BUG] Fix cloning of config for nested objects

[tpvasconcelos]

Reference Issues/PRs

Fixes #276

What does this implement/fix? Explain your changes.

Please refer to: #276

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

The _clone() method has a # copied from sklearn comment above it. Any concerns with modifying this method? Any alternative solutions come to mind?

PR checklist

For all contributions

• I've reviewed the project documentation on contributing
• I've added myself to the list of contributors.
• The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
  the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.

For code contributions

• Unit tests have been added covering code functionality
• Appropriate docstrings have been added (see documentation standards)
• New public functionality has been added to the API Reference

[fkiraly]

Astutely observed.

However, question, given that there are two points of recursion

• the first, this deals with nesting in lists etc.
• the second, new_object_params[name] = self._clone(param, safe=False), this deals with direct nesting.

Naively, I would think we do not need to change the first occurrence, current line 228, because we go back to _clone and from there to line current 250.

Further, non-blocking question, I was wondering: should we make the contract less strict about inheritance, and more about interfaces, would it make sense to do hasattr(param, "clone") and hasattr(param, "_config")? Or perhaps it is safer to make the deviation from sklearn more strict, which is the current state. I tend towards the latter.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b754e93) 84.69% compared to head (c6cdd7f) 84.73%.

❗ Current head c6cdd7f differs from pull request most recent head 35ffcf8. Consider uploading reports for the commit 35ffcf8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
+ Coverage   84.69%   84.73%   +0.03%     
==========================================
  Files          43       43              
  Lines        2928     2928              
==========================================
+ Hits         2480     2481       +1     
+ Misses        448      447       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tpvasconcelos]

Naively, I would think we do not need to change the first occurrence, current line 228, because we go back to _clone and from there to line current 250.

I changed the implementation to just do the config cloning inside the _clone method instead of having it in the clone method. I think this is a cleaner implementation.

[tpvasconcelos]

Further, non-blocking question, I was wondering: should we make the contract less strict about inheritance, and more about interfaces, would it make sense to do hasattr(param, "clone") and hasattr(param, "_config")? Or perhaps it is safer to make the deviation from sklearn more strict, which is the current state. I tend towards the latter.

With the current implementation (see latest changes) this is not relevant anymore. Could you take a look? 🙏

[fkiraly]

Very elegant!

Putting the set_config at the recursion nodes rather than having it just in the top node solves the problem conclusively and elegantly. Nice.

Cohesion is maintained since the total number of calls does not change, and _clone gets to handle set_params and set_config, so even that is improved.

[fkiraly]

You should also add yourself here: https://github.com/sktime/skbase/blob/main/.all-contributorsrc

bug, code, ideas.

[fkiraly]

The _clone() method has a # copied from sklearn comment above it. Any concerns with modifying this method?

Since you are asking: in-principle no concerns. The history behind _clone is that we originally wanted a clean re-implementation which turned out to be discrepant from sklearn in some important examples due to the magic after if estimator_type in (list, tuple, set, frozenset): and safe arg which defines a very specific contract.

Ultimately, one high-level compatibility/interoperability requirement for BaseObject is that BaseEstimator could replace sklearn BaseEstimator with at most minimal adjustments (all of which must be improvements), so the only quick way to get there, after multiple unsuccessful attempts at re-implementation, was to copy-paste sklearn clone.

Funnily, sklearn seem to be going in a similar direction by allowing estimators to dispatch to their internal clone (which seems a bit unsafe to me compared to base class inheritance). Either way, some tech discussions on uniformization of APIs are going to be scheduled for later this year between the two projects (sktime, sklearn) - in case you are interested.

[tpvasconcelos]

I see...

Just for reference, I'm attaching a visual diff between the current BaseObject._clone() implementation and sklearn's _clone_parametrized():

Summary of differences:

1. skbase does not clone dict values (@fkiraly do you know why?)
2. sklearn also clones the _metadata_request and _sklearn_output_config attributes
3. skbase now also clones the config flags

I need to think more about this to give any meaningful input here.

Either way, some tech discussions on uniformization of APIs are going to be scheduled for later this year between the two projects (sktime, sklearn) - in case you are interested.

@fkiraly definitely