@glouppe @arjoly Could you please verify if this is a bug or not? I do not know the internals of boosting to comment on whether the sample_weights provided to the fit in the meta-estimator is the same as that passed to fit in the base estimators. But by how it is implemented in AdaBoostClassifier, it seems like that.

Interesting. I am encountering a failed test in this PR: #5204 from within test_sample_weight_missing() of ensemble/tests/test_weight_boosting.py because it uses an AdaBoostRegressor with LogisticRegression and not raising the ValueError. Connected?

Yes :) It might be better to keep the changes independent though, i.e wait till this PR is merged and rebase the sample_weights PR over this.

What is test_sample_weight_missing meant to check for? I think the parts of that test which use LogisticRegression can be removed in #5204 since sample_weight would now be a parameter?

I think the idea is to raise an error or not support base estimators which do not have sample_weights. However, I suppose an error should be raised only when sample_weights is not None for these cases?

Anyhow, the checks for LogisticRegression can be removed in the other PR or the solver can be explicitly set to liblinear since that should also raise a ValueError (although in a different part of the code)

Actually this is not a bug. The weighting of samples is taken care by the block of code above which does a weighted random sampling of n_samples, hence we need not pass the sample_weights.

AdaBoostRegressor should work without the base estimator needing sample_weights.

@vstolbunov I've updated the pull request. Let me know what you think.

Good point. Looks good to me.

I dont understand this PR. AdaBoostRegressor needs the base estimator to use the sample weights that are computed at each iteration (see https://github.com/MechCoder/scikit-learn/blob/ensemble_sample_weight_bug/sklearn/ensemble/weight_boosting.py#L1053 for regression). Therefore the base estimator must support sample weights.

I am confused, but in the code over the base estimator does not require these weights to be passed to the fit method over here (https://github.com/MechCoder/scikit-learn/blob/ensemble_sample_weight_bug/sklearn/ensemble/weight_boosting.py#L1020) and the handling of sample_weights is externally done from L1011 TO L1016 right?. Where else is it passed to the base estimator?

Indeed, that is strange. I think this was done on purpose but I am not sure anymore. @ndawe do you remember?

Are you okay with removing the sample_weights check for now (as done in this PR)? and opening an issue that discusses this for the future?

This check for sample_weight support was not in the original implementation that I wrote (as I recall). Git blame shows @akshayah3. For regression, sample_weight support is not required in the base estimator since AdaBoost.R2 only uses the weights in the bootstrapping. So this check should be removed.

@ndawe Thanks! Would you be able to do a quick code review? This is blocking another LogisticRegression pr

+1

Just had a quick look and this looks fine to me.

Any second reviewers.
ping @glouppe @arjoly

Hope this gets passed soon so we can have both #5207 and #5204 in the next release! :)

ping @mblondel from #3711.

I don't see why we shouldn't just merge this right now :)

I'll leave the bold step of doing this to others :)

Sorry for the late review. I just checked the paper, this is indeed the way it is supposed to be for AdaBoost.R2.

So was #3711 just a mistake?

Yes, it shouldn't have been added for the regressor. (But this was correct for the classifier.)

Thanks @MechCoder !