skbase icon indicating copy to clipboard operation
skbase copied to clipboard

[ENH] should `set_config` and `get_config` be able to address `deep` configs?

Open fkiraly opened this issue 1 year ago • 5 comments

here's a question: should there be a deep arg to get_config, like get_params? That way, we could avoid changes to _clone, as then we just need to add the deep arg in clone, once. It would be "cleaner" imo since it completely decouples the param logic from the config logic (coupling/cohesion improvement).

That is, should get_config, set_config of the top node object support getting/setting of component node configs - like get_params, set_params - or do you have to address the components directly.

I.e., compare

# how it would work right now
composite = Composite(comp1=Component1(), comp2=Component2().set_config(foo="bar"))
# vs how it could also work
composite = Composite(comp1=Component1(), comp2=Component2())
composite.set_config(comp2__foo="bar")

Both ways are already making the assumption that clone should keep configs across all nodes.

Originally posted by @fkiraly in https://github.com/sktime/skbase/issues/276#issuecomment-1918212295, moved here for discussion.

fkiraly avatar Jan 31 '24 23:01 fkiraly

A reasonable question. Having a way to set config on nested objects seems to be needed. I like composite.set_config(comp2__foo="bar") as an interface.

A related question, should set_config have a recursive option, or a recursive implementation. e.g.

composite = Composite(comp1=Component1(), comp2=Component2())
composite.set_config(foo="bar", recursive=True)
# or
composite.set_config_recursive(foo="bar")

Where this would set foo="bar" on all BaseObjects accessible from composite. The current use case would be to be able to set remember_data=False on all Estimators nested in an object in one call.

wilsbj avatar Apr 05 '25 09:04 wilsbj

A related question, should set_config have a recursive option, or a recursive implementation.

Interesting, I like this idea!

Maybe this is overengineering, but tags could be by default recursive or not, user expectations will differ here. Perhaps that is only something for downstream packages, to avoid overcomplicating the interface.

fkiraly avatar Apr 07 '25 08:04 fkiraly

other, more mundane question, @wilsbj - would it be better to call this recursive, or deep? To be in line with get_config. However, this might be confusing because the "deep" write - the counterpart to the deep param of get_params - is automatic in set_params through the __ syntax.

Or is it not confusing, because deep then consistently means "impact all components"?

fkiraly avatar Apr 07 '25 09:04 fkiraly

I initially though deep would be appropriate here, but deep in set_params only intends to set a given param in one place, while this idea is to set the same config in multiple places.

wilsbj avatar Apr 07 '25 10:04 wilsbj

@wilsbj, there will be no deep in set_params which indicates which parameters are accessed, since it is superfluous. Please let me know if you think I am wrong.

There is no need to pass on the "deep yes/no" information to set_params in an extra argument, as it is already done via the keys:

  • set_params(**{"sthsth": value}) - not deep
  • set_params(**{"sthsth__sthelse": value}) - deep

Therefore, the name deep would be free to mean something else, e.g., "recursive".

fkiraly avatar Apr 07 '25 10:04 fkiraly