✔️ Linting Passed

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

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

Thanks for the PR. Any reason to decide to introduce a new check_estimator_sparse_tagestimator check instead of making the existing check_estimator_sparse_array tag-aware?

Since estimator checks are already costly to run, I would rather avoid proliferation and redundancy when possible.

I tried to make check_estimator_sparse_array tag-aware, but didn't manage to combine the old behavior (testing on various sparse formats, and if the estimator doesn't support the sparse format checking that it fails gracefully with the appropriate error) and what the new test is doing (checking that sparse tag is equivalent to raising or not the error).

Actually the new test is done with the assumption that the sparse flag is equivalent to accept csr input. It seems that the csr format is kind of a default (if the estimator can accept sparse input it always include csr, but it may reject other sparse formats), I didn't encounter a counterexample in the estimators.

Perhaps a cleaner alternative as suggested by @glemaitre would be that

1. the sparse tag is the list of supported sparse formats instead of a boolean
2. make validate_data rely on the sparse estimator tag instead of the accept_sparse argument.

Perhaps a cleaner alternative as suggested by @glemaitre would be that
the sparse tag is the list of supported sparse formats instead of a boolean
make validate_data rely on the sparse estimator tag instead of the accept_sparse argument.

I am a bit afraid about the breaking change induced by entirely changing the meaning of the .input_tags.sparse tag but if that many estimators needed a fix in the first place, it probably means that nobody was actually relying on that tag anywhere within our outside the scikit-learn code base anyway so we are free to make the changes we want.

+1 for updating your PR to implement what you suggest above and see what it practically entails.

@antoinebaker do not hesitate to ping me when you want another review.

I followed @glemaitre suggestion for setting the sparse tag in Pipeline.

We now definesparse = all steps accept sparse instead of the previous one sparse = first non passthrough step accepts sparse. Both are incorrect, but determining the correct flag seems a nightmare in general as some transformers may change the sparse nature of the data.

As a lesser of two evils the new definition will have false negatives (Pipelines accepting sparse data wrongly tagged sparse=False) while the old one had false positives (Pipelines rejecting sparse data wrongly tagged sparse=True).

Also, I think we should include it in 1.6.1 because it fixes tags that we just made public

I think there are still things to improve with the sparse tag but it can be done in a separate PR. I'm in favor of merging this one as is to not delay the 1.6.1 release further.

Yep it is indeed better than what we had. We can later improve and discussed having a ternary option instead of the binary one.

Thanks @antoinebaker. It was a big one ;)