✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 49f991b. Link to the linter CI: here}

The test split looks good. Note that some of them are already failing with the new X for the current test only setting some weights to zero.

The failures are not surprising for some of them, e.g. Perceptron, because they are stochastic. It does not mean that they don't correctly handle sample weight but that we need to test in expectation.
Looking at this https://gist.github.com/snath-xoc/fb28feab39403a1e66b00b5b28f1dcbf, it looks like Perceptron doesn't handle sample weight correctly anyway though :)

The other ones will need a case by case investigation.

I'm okay with marking them as xfail for now and adding them to the list of known failures in #16298. Then we'll try to fix them one by one.

I think we should add a changelog entry in doc/whats_new/v1.6.rst. For now we can add it under the utils.estimator_checks module. Since they Developer API is going through significant refactoring we might move this changelog entry to a dedicated "Developer API" later but I think using a module section named utils.estimator_checks is fine for the time being.

If needed we could introduce a new tag for estimator for which fitting on the same data but changing the value of random_state is expected to have a significant impact on the fitted attributes and/or output of predictions. This way we could skip this check instead of xfailing it (e.g. for perceptron).

However, I have the feeling that this tag might be complex to set. It might depend on other hyper-parameters (e.g. a choice of a solver, a choice of max_features... or even the value of tol, max_iter, and the data when the objective function is convex...). So let's stick to xfailing for now :)

If needed we could introduce a new tag for estimator for which fitting on the same data but changing the value of random_state is expected to have a significant impact on the fitted attributes and/or output of predictions. This way we could skip this check instead of xfailing it (e.g. for perceptron).

However, I have the feeling that this tag might be complex to set. It might depend on other hyper-parameters (e.g. a choice of a solver, a choice of max_features... or even the value of tol, max_iter, and the data when the objective function is convex...). So let's stick to xfailing for now :)

Isn't every estimator which as a random_state expected to give different results for different seeds? I don't think it matters how significant the differences between seeds are, they're expected to be different, and we always set the random state for estimators in our tests for this reason. That's why even if setting the flag was easy, I'd be hesitating about what it adds.

Isn't every estimator which as a random_state expected to give different results for different seeds? I don't think it matters how significant the differences between seeds are, they're expected to be different, and we always set the random state for estimators in our tests for this reason.

Sometimes it depends on the other parameters: for instance, for logistic regression, fitting with the default solver ("lbfgs") is deterministic but fitting with "saga" and "sag". However, if you set tol to be low enough and max_iter to be large enough, then even "SAGA" and "SAG" become deterministic because the loss is convex and they converge to the unique solution, irrespective of per-iteration random data shuffling.

we always set the random state for estimators in our tests for this reason.

Note that for stochastic estimator, the repetition vs reweighing strict equivalence will not hold in general even when we set the same seed for the repeated and the reweighed estimator because the rng state does not have the same meaning with a different number of data points (e.g. whenever the data is permuted or resampled). Hence the need for statistical testing to detect sample_weight related bugs for stochastic estimators.

The reason it was triggered in test_check_estimator and not test_common, is that one uses parametrized_with_checks and the other check_estimator.

Is this behavior of check_estimator something that we want? Do we want to make XFAIL-handling of exceptions a publicly documented feature of check_estimator. That would probably require upgrading/renaming _xfail_checks to xfail_checks to make it a public estimator tag.

Maybe the default behavior could be to let exceptions go through (xfailed_checks="raise") but also offer xfailed_checks="skip" and xfailed_checks="warn" as alternatives?

The docstring of check_estimator could explain that if the user wants pytest to handle XFAIL cases the usual way, they should rather use parametrize_with_checks instead of calling check_estimator in a test function.

EDIT: I had not realized that d525b68 has already implemented xfailed_checks="skip" which is good enough for now. We can implement the other suggestions in follow-up PRs.

Any second review @jeremiedbb @adrinjalali?

Thanks for the review @jeremiedbb ! We indeed hesitated between keeping the sample_weight = ones test for the reason you mention (easy debugging if it fails) and removing it. Maybe we could add it a "utility" test when developing an estimator/transformer but not add it to the test suite, not sure where it should belong in the code ?

I think the new test is more enough, and adding the "ones" case moreover would bring an unfavorable "test sensitivity" vs "test CI cost" trade off.

Maybe we could add it a "utility" test when developing an estimator/transformer but not add it to the test suite, not sure where it should belong in the code ?

Let's wait to see one issue or PR where such a utility would have helped. If we need it, we code it and add it back as a dedicated estimator check. But I suspect we will never need it.