did we discuss what we're going to do with this api-wise?

did we discuss what we're going to do with this api-wise?

For now the plan is to follow #3855 (comment). The unclear part for the API is how array-like fit parameters, such as sample_weight will be handled, since they will need to be resampled in pipelines.

If you use sample_props dictionary to contain properties mapped to the samples (eg sample_weight) the pipeline implementation for resampling transformers can be as follows:

for step_idx, (name, transformer) in enumerate(self.steps[:-1]):

    fit_params_step = {key.split('__', 1)[1]: fit_params.pop(key) for key in fit_params if name + "__" in key};

    if isinstance(transformer, ResamplerMixin):
        props = {key : fit_params[key] for key in sample_props}
        X, y, props = transformer.fit_resample(X, y, **fit_params_step, props)
        fit_params.update(props)


    else: ... # is a regular transfomer

Since we didn't get to discuss it today, here's a review of (by my understanding) the current plan for sample_props:

• introduce a parameter to resamplers named sample_props of type dict string -> (array-like, shape=(n_samples,))
• resamplers take sample_props as an argument to fit_resample. Upon resampling, they accordingly resample the arrays in sample_props.
• resamplers return X, y, sample_props if sample_props were passed. If no sample_props were passed, the resampler returns X, y.
• pipeline will, while fitting, check the args of the estimator/transformer it is fitting. If a parameter arg is both a fit parameter and in sample_props, the pipeline will pass sample_props[arg] to fit/fit_transform.
• furthermore, pipeline will correctly pass along modified sample_props when resamplers are in the pipeline.

Let me know if I'm understanding something wrong.

I think a cleaner / simpler way to do it would be to specify which fit_params are sample_props. this can be accomplished using a list of keys (as per the implementation I have above). But I think your way works too.

Also I think it's better to have a consistent return signature, and its ok to return None for sample_props if none were passed into the resampler.

I don't think we want a parameter named sample_props, but that's unresolved. In my mind, these props are exactly what we have as sample_weight and groups now. I also think (but not sure) a resampler should be permitted to return props (weights especially) without any being passed in. I'd be tempted to say that resamplers should always generate (X, y, dict of props) but maybe we should initially propose and handle just the (X, y) case. Sorry I've not yet reviewed this.

This is a really good point - a really clean solution would be to check fit_params for items that are sample_props (have len = len(X)). This avoids passing multiple dictionaries / parameter collections.

After the discussion, I think it would be better to not implement sample_props in this PR (I was not aware that they would be such an API-wide change).

Ok so I will review tomorrow and start to make a SLEP or update one that is opened. We can check that together.

I discussed this briefly with Guillaume last night. As much as it would be great to just try to merge this, I think we now understand a few things about it (e.g. not supporting fit_transform, or transform in the same object; sample_prop output and hence wanting to support alternatively 2- or 3-tuple output) that are worth describing formally and getting wider review.

describing formally and getting wider review

I could write a draft SLEP if that's useful (or update an existing one).

@jnothman I've added a function check_X_y_kwargs that validates kwargs along with X and y. Since you said all estimators should have fit kwargs of shape (n_samples,), maybe this is something that should be applied across the codebase?

Note that I also found some estimators (KernelRidge, Ridge) that accept float as sample_weight. So this check would break for them.

@orausch is the Pull Request still active? This is a feature which I dearly need in Sklearn. I'm on the edge of having implement it myself in a separate project but, obviously, helping out put it into Sklearn would be MASSIVELY preferable!

@jnothman I've heard rumblings that SLEP001 is preferable?

@Jude188 the work here is stalled since we had some trouble reaching a decision in scikit-learn/enhancement_proposals#15. I think the next steps would be to sort the SLEP out and get it approved; this PR was more of an exploration/POC anyway.

@Jude188 You can look at imbalanced-learn which provide compatible tools in the meanwhile

@orausch, will you be available to continue this, or should we consider passing it onto someone else?

@jnothman I'm happy to contribute some more to this, but the next steps/goals are currently a bit unclear to me (especially because it has been hard to reach consensus in the SLEP). Of course its also easier to invest time with a clear plan/requirements for merging.

Do you have any particular goals in mind?