search

scikit-learn / enhancement_proposals

Enhancement proposals for scikit-learn: structured discussions and rational for large additions and modifications
https://scikit-learn-enhancement-proposals.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
48 stars 34 forks source link

SLEP006 (sample props) should handle when metaestimator consumes the same key as its descendant #58

Closed jnothman closed 1 year ago

jnothman commented 3 years ago

From https://github.com/scikit-learn/scikit-learn/pull/21284#discussion_r728109717:

The case of a metaestimator adding support for a prop that is requested by its child is indeed a tricky one. I can't yet see a way to make this generally backwards compatible within the SLEP006 proposal. This makes me sad.

Indeed, generally a metaestimator supporting the same prop name as one of its children is tricky. I.e. if the metaestimator supports metadata x and its child requests metadata x, the metaestimator should only work where either:

  • the child's request aliases x to another name without such a clash;
  • the child's request and the metaestimator's request for x implies being passed the same keyword argument.

In other cases, this must raise an error. This is something, I'm pretty sure, we've not yet covered in SLEP006 (and it's a pretty messy and intricate consequence of having the caller responsible for delivering metadata in accordance with the request).

Deprecation would be pretty tricky as far as I can tell.

adrinjalali commented 3 years ago

Right now we have {meta}estimators which have fit(X, y, **fit_params) and they pass all fit_params to the underlying estimator. If this meta-estimator is to support sample_weight, and the underlying estimator does too, then we would just use sample_weight and also pass it down to the sub-estimator. This is the current API design, and SLEP006 doesn't really change that.

jnothman commented 3 years ago

If the meta-estimator has a request to pass the thing called 'sample_weight' to its child, then if it supports sample_weight itself, there is no way to disable it using that sample weight, without the user modifying the child's request to use another alias. Does that make sense?

adrinjalali commented 3 years ago

Yes, but the current behavior is the same, SLEP006 at least gives the user and the devs to allow user to disable sample_weight consumption in the meta-estimator.

adrinjalali commented 2 years ago

Are you happy with what we've got? Can we close this one?