Talip7/scikit-learn-issues-multilabel huggingface dataset:
HistGradientBoosting counts and sample weights

Related issues: #25210

### Current State
`HistGradientBootingClassifier` and `HistGradientBootingRegressor` both:
- Calculate the sample size `count` in histograms
- Use `count` for splitting (mostly excluding split candidates)
- Save the `count` in the final trees and use it in partial dependence computations.

### Proposition
1. Evaluate if removing `count` from the histograms (LightGBM only sums gradient and hessian in histograms, no count) gives a good speed-up .(I measured a roughly 10-20% speed-up.)
  Edit: LightGBM uses an approximate count based on the hessian to check for min sample size. So this might not be what we want.
3. Add an option to save counts and sample weights to final trees at the very end of `fit` (where the binned training `X` is still available).
4. Use partial dependence `method='recursion'` if the above option was set, else use `method='brute'`.

Why?
#25431 concluded that adding weights to the trees is too expensive. The above proposition gives a user a clear choice: Faster training time or faster pdp afterwards.

COMMENT:
@vishalj0501 this is a very hard issue requiring familiarity with the codebase. I suggest you start with issues marked as "good first issue"

Labels: New Feature, Performance, Needs Decision, Needs Benchmarks, module:ensemble, Breaking Change
HalvingRandomSearchCV and HalvingGridSearchCV do not support multimetric scoring.

According to https://scikit-learn.org/stable/modules/grid_search.html, 

_HalvingRandomSearchCV and HalvingGridSearchCV do not support multimetric scoring._

When will this be implemented?

When trying to use multimeric scoring today, I get: 

`ValueError: scoring parameter must be a string, a callable or None. Multimetric scoring is not supported.`

COMMENT:
For context on why multimetric scoring isn't supported for now: https://github.com/scikit-learn/scikit-learn/pull/13900#discussion_r318063512

Labels: New Feature, module:model_selection, Needs Decision - Include Feature
HalvingRandomSearchCV and HalvingGridSearchCV do not support multimetric scoring.

According to https://scikit-learn.org/stable/modules/grid_search.html, 

_HalvingRandomSearchCV and HalvingGridSearchCV do not support multimetric scoring._

When will this be implemented?

When trying to use multimeric scoring today, I get: 

`ValueError: scoring parameter must be a string, a callable or None. Multimetric scoring is not supported.`

COMMENT:
@NicolasHug @jnothman do you know when we can expect a fix?

Labels: New Feature, module:model_selection, Needs Decision - Include Feature
Performance regression in KMeans.transform

https://scikit-learn.org/scikit-learn-benchmarks/#cluster.KMeansBenchmark.time_transform?p-representation='dense'&p-algorithm='elkan'&p-init='k-means%2B%2B'&commits=ab7e3d19-b397b8f2

The increment is quite small in absolute value but quite consistent so it could be worth investigating what could have caused it (between ab7e3d19 and b397b8f2).

COMMENT:
Looking at the [diff between those two commits](https://github.com/scikit-learn/scikit-learn/compare/ab7e3d1...b397b8f) there are two things that could be relevant. There is no obvious change.

The lockfiles were updated but I doubt this is what caused this.

There was also the change by @jeremiedbb to introduce `IntNotReal` for parameter validation. However it isn't used directly in `KMeans`. Do you think it is worth investigating this aspect further Jeremie?

Labels: Performance, Regression
Performance regression in KMeans.transform

https://scikit-learn.org/scikit-learn-benchmarks/#cluster.KMeansBenchmark.time_transform?p-representation='dense'&p-algorithm='elkan'&p-init='k-means%2B%2B'&commits=ab7e3d19-b397b8f2

The increment is quite small in absolute value but quite consistent so it could be worth investigating what could have caused it (between ab7e3d19 and b397b8f2).

COMMENT:
A profiling is necessary, but I strongly suspect that it comes from https://github.com/scikit-learn/scikit-learn/pull/25864 which added validation on ``gen_batches`` used many times in ``euclidean_distances`` on float32. This is another motivation for https://github.com/scikit-learn/scikit-learn/pull/25815

Labels: Performance, Regression
Memory usage regression in RandomForestClassifier.fit on sparse data

There was an old (around the time of the 1.1 release) memory usage regression in `RandomForestClassifier.fit`:

- https://scikit-learn.org/scikit-learn-benchmarks/#ensemble.RandomForestClassifierBenchmark.peakmem_fit?p-representation='sparse'&p-n_jobs=4&commits=e275d9df-5436818d

Between e275d9df and 5436818d.

Given the magnitude of the regression, it could be caused by an unwanted extra data copy.

COMMENT:
I will have a look at this one.

Labels: Performance, Regression
Memory usage regression in RandomForestClassifier.fit on sparse data

There was an old (around the time of the 1.1 release) memory usage regression in `RandomForestClassifier.fit`:

- https://scikit-learn.org/scikit-learn-benchmarks/#ensemble.RandomForestClassifierBenchmark.peakmem_fit?p-representation='sparse'&p-n_jobs=4&commits=e275d9df-5436818d

Between e275d9df and 5436818d.

Given the magnitude of the regression, it could be caused by an unwanted extra data copy.

COMMENT:
I don't succeed to reproduce the issue. I used the following script coming that uses the setup from the ASV benchmark:

```python
from benchmarks.ensemble import RandomForestClassifierBenchmark

param = ("sparse", 4)
bench = RandomForestClassifierBenchmark()
X_train, X_test, y_train, y_test = bench.make_data(param)
estimator = bench.make_estimator(param)


@profile
def fit_estimator():
    estimator.fit(X_train, y_train)


if __name__ == "__main__":
    fit_estimator()

```

I get the following profile once the data are cached:

On `main`:

![profiling_main](https://user-images.githubusercontent.com/7454015/230383606-b320df8d-d860-4d03-a491-2f8a647cce59.png)

On `1.0.2`:

![profiling_1_0_2](https://user-images.githubusercontent.com/7454015/230383611-4a44344f-90bf-4202-8002-b3f5bbb5109d.png)

I observed the +30MiB that should come from the check for extension arrays. However, I don't observe the +200MiB.

I will try to reduce the interval in case the copy is quickly garbage collected.

Labels: Performance, Regression
Memory usage regression in RandomForestClassifier.fit on sparse data

There was an old (around the time of the 1.1 release) memory usage regression in `RandomForestClassifier.fit`:

- https://scikit-learn.org/scikit-learn-benchmarks/#ensemble.RandomForestClassifierBenchmark.peakmem_fit?p-representation='sparse'&p-n_jobs=4&commits=e275d9df-5436818d

Between e275d9df and 5436818d.

Given the magnitude of the regression, it could be caused by an unwanted extra data copy.

COMMENT:
Edit: I tried with a 10ms interval:

On `main`:

![profiling_main](https://user-images.githubusercontent.com/7454015/230387078-575ee951-4b45-4f82-ac41-3a1b5cc9629e.png)

On `1.0.2`:

![profiling_1_0_2](https://user-images.githubusercontent.com/7454015/230387045-5c948cdd-c6dc-49e1-bf78-56e545d36073.png)

I still don't see the +200MiB.

Labels: Performance, Regression
Memory usage regression in RandomForestClassifier.fit on sparse data

There was an old (around the time of the 1.1 release) memory usage regression in `RandomForestClassifier.fit`:

- https://scikit-learn.org/scikit-learn-benchmarks/#ensemble.RandomForestClassifierBenchmark.peakmem_fit?p-representation='sparse'&p-n_jobs=4&commits=e275d9df-5436818d

Between e275d9df and 5436818d.

Given the magnitude of the regression, it could be caused by an unwanted extra data copy.

COMMENT:
Alright, thanks for attempting to reproduce it. Let's close for now and let's reopen of someone manage to reproduce it.

It might be caused by change of behavior in one of our dependencies instead of our code.

Labels: Performance, Regression
Memory usage performance regression in several scikit-learn estimators

There is a weird yet significant memory usage increase in many seemingly unrelated scikit-learn estimators that happened on 2023-03-22, e.g.:

- https://scikit-learn.org/scikit-learn-benchmarks/#neighbors.KNeighborsClassifierBenchmark.peakmem_fit?p-algorithm='brute'&p-dimension='low'&p-n_jobs=4&commits=e3d1f9ac-18af5508

- https://scikit-learn.org/scikit-learn-benchmarks/#linear_model.LogisticRegressionBenchmark.peakmem_predict?p-representation='sparse'&p-solver='saga'&p-n_jobs=1&commits=e3d1f9ac-18af5508

It would be necessary to inspect what change between e3d1f9ac and 18af5508 could be responsible for this.

COMMENT:
It seems that there is a fixed ~30 MB memory usage increment.

Labels: Performance, Regression
Performance regression in pairwise_distances with the Euclidean metric on sparse data

As spotted by our continuous benchmark suite, there is a more than 2x slowdown in `pairwise_distances` on sparse input data (for the Euclidean metric).

- https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=b4afbeee-fabe1606

This happened between b4afbeee and fabe1606.

Could it be ef5c0879e1367397f671791cdfad46eef8892826?

It's not sure because there are many other commits in `git log b4afbeee..fabe1606` and I did not review them all.

COMMENT:
I guess it's the same reason as for KMeans.transform explained here https://github.com/scikit-learn/scikit-learn/issues/26100.

Labels: Performance, Regression
Performance regression in pairwise_distances with the Euclidean metric on sparse data

As spotted by our continuous benchmark suite, there is a more than 2x slowdown in `pairwise_distances` on sparse input data (for the Euclidean metric).

- https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=b4afbeee-fabe1606

This happened between b4afbeee and fabe1606.

Could it be ef5c0879e1367397f671791cdfad46eef8892826?

It's not sure because there are many other commits in `git log b4afbeee..fabe1606` and I did not review them all.

COMMENT:
I cannot reproduce the slowdown between `1.2.1` and `main`:

```python
from benchmarks.common import Benchmark
from benchmarks.metrics import PairwiseDistancesBenchmark

Benchmark.data_size = "large"

params = ("sparse", "euclidean", 4)
bench = PairwiseDistancesBenchmark()
bench.setup(*params)


def time_pairwise_distances():
    bench.time_pairwise_distances()


if __name__ == "__main__":
    time_pairwise_distances()

```

And here are the two profiles for `1.2.1` and `main`, respectively. Time-wise, it takes the same time.

![profile_1_2_1](https://user-images.githubusercontent.com/7454015/230395972-c4d5d545-850e-4466-b33a-faeb57b09520.svg)

![profile_main](https://user-images.githubusercontent.com/7454015/230395980-ff39e7f9-398c-4ced-a6dd-b0182d8d0228.svg)

Labels: Performance, Regression
Performance regression in pairwise_distances with the Euclidean metric on sparse data

As spotted by our continuous benchmark suite, there is a more than 2x slowdown in `pairwise_distances` on sparse input data (for the Euclidean metric).

- https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=b4afbeee-fabe1606

This happened between b4afbeee and fabe1606.

Could it be ef5c0879e1367397f671791cdfad46eef8892826?

It's not sure because there are many other commits in `git log b4afbeee..fabe1606` and I did not review them all.

COMMENT:
After some digging, I'm not so sure that it's due to the validation of `gen_batches`. The dimensions of the dataset used in the benchmark is such that `gen_batches` is call ~10 times. With ~50us per call, it should not be visible at all in a 2s run.

Labels: Performance, Regression
Performance regression in pairwise_distances with the Euclidean metric on sparse data

As spotted by our continuous benchmark suite, there is a more than 2x slowdown in `pairwise_distances` on sparse input data (for the Euclidean metric).

- https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=b4afbeee-fabe1606

This happened between b4afbeee and fabe1606.

Could it be ef5c0879e1367397f671791cdfad46eef8892826?

It's not sure because there are many other commits in `git log b4afbeee..fabe1606` and I did not review them all.

COMMENT:
I finally found the commit responsible for the regression: it's [de67a44](https://github.com/scikit-learn/scikit-learn/commit/de67a4420f1713058070802ad593cbcd2ee2d5f3) from PR https://github.com/scikit-learn/scikit-learn/pull/25598/

I ran the benchmarks for this commit and the commit just before which confirms that: https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=de67a442

The reason for the regression is another occurence of https://github.com/xianyi/OpenBLAS/issues/3187. We're running ``row_norms`` (which now uses openmp) and ``matmul`` (which uses BLAS) sequentially in a loop. It happens when BLAS and openmp don't share the same threadpool, i.e. when the BLAS uses pthread (e.g. OpenBLAS from PyPI) or a different openmp (e.g. MKL uses intel-openmp).

@glemaitre, the reason you couldn't reproduce is probably because on macos your install is such that the BLAS and scikit-learn use the same openmp lib.

Unfortunately I don't know any easy fix for that besides reverting this commit.

Labels: Performance, Regression
Performance regression in pairwise_distances with the Euclidean metric on sparse data

As spotted by our continuous benchmark suite, there is a more than 2x slowdown in `pairwise_distances` on sparse input data (for the Euclidean metric).

- https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=b4afbeee-fabe1606

This happened between b4afbeee and fabe1606.

Could it be ef5c0879e1367397f671791cdfad46eef8892826?

It's not sure because there are many other commits in `git log b4afbeee..fabe1606` and I did not review them all.

COMMENT:
Thanks for the investigation.  Indeed i don't see any short term solution beyond advising our users to install packages from conda-forge. In the longer term we will need to come back the possibility to ship numpy/scipy wheels that somehow rely on an openblas linked to a specific vendored openmp for each supported OS.

Labels: Performance, Regression
Performance regression in pairwise_distances with the Euclidean metric on sparse data

As spotted by our continuous benchmark suite, there is a more than 2x slowdown in `pairwise_distances` on sparse input data (for the Euclidean metric).

- https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=b4afbeee-fabe1606

This happened between b4afbeee and fabe1606.

Could it be ef5c0879e1367397f671791cdfad46eef8892826?

It's not sure because there are many other commits in `git log b4afbeee..fabe1606` and I did not review them all.

COMMENT:
I am +1 on reverting for now.

Moving forward, we'll need to be more careful when merging PRs that add OpenMP. Currently the best way to catch the issue is to run the benchmarks with a SciPy installed on PyPI.

Labels: Performance, Regression
Performance regression in pairwise_distances with the Euclidean metric on sparse data

As spotted by our continuous benchmark suite, there is a more than 2x slowdown in `pairwise_distances` on sparse input data (for the Euclidean metric).

- https://scikit-learn.org/scikit-learn-benchmarks/#metrics.PairwiseDistancesBenchmark.time_pairwise_distances?p-representation='sparse'&p-metric='euclidean'&p-n_jobs=4&commits=b4afbeee-fabe1606

This happened between b4afbeee and fabe1606.

Could it be ef5c0879e1367397f671791cdfad46eef8892826?

It's not sure because there are many other commits in `git log b4afbeee..fabe1606` and I did not review them all.

COMMENT:
An alternative is to make row_norms able to use a float64 accumulator. This way we don't have to call row_norms for each chunk in _euclidean_distances_upcast. I created a branch to try that, but since row_norms is public and used in many places it requires more work. So I think we should revert for now (I opened #26275) and possibly come back later on this alternative.

Labels: Performance, Regression
Rename quantile parameter in QuantileRegressor and HistGradientBoostingRegressor

[`QuantileRegressor`](https://scikit-learn.org/dev/modules/generated/sklearn.linear_model.QuantileRegressor.html#sklearn.linear_model.QuantileRegressor) and [`HistGradientBoostingRegressor`](https://scikit-learn.org/dev/modules/generated/sklearn.ensemble.HistGradientBoostingRegressor.html#sklearn.ensemble.HistGradientBoostingRegressor) both have a `quantile` parameter. But the meaning is the **probability level** of the quantile to be estimated/predicted, not the quantile itself.

R's [`quantile`](https://stat.ethz.ch/R-manual/R-devel/library/stats/html/quantile.html) function calls this parameter `probs` for probability.

Can we consider to rename it? Possible suggestions:
- `quantile_level`
- `probability`

COMMENT:
I don't like `probability`. Maybe `quantile_level` is fine.

Labels: API, Needs Decision, module:ensemble, module:linear_model
Rename quantile parameter in QuantileRegressor and HistGradientBoostingRegressor

[`QuantileRegressor`](https://scikit-learn.org/dev/modules/generated/sklearn.linear_model.QuantileRegressor.html#sklearn.linear_model.QuantileRegressor) and [`HistGradientBoostingRegressor`](https://scikit-learn.org/dev/modules/generated/sklearn.ensemble.HistGradientBoostingRegressor.html#sklearn.ensemble.HistGradientBoostingRegressor) both have a `quantile` parameter. But the meaning is the **probability level** of the quantile to be estimated/predicted, not the quantile itself.

R's [`quantile`](https://stat.ethz.ch/R-manual/R-devel/library/stats/html/quantile.html) function calls this parameter `probs` for probability.

Can we consider to rename it? Possible suggestions:
- `quantile_level`
- `probability`

COMMENT:
can I handle this Issue? I think this issue is suited for beginners like me.

Labels: API, Needs Decision, module:ensemble, module:linear_model
Rename quantile parameter in QuantileRegressor and HistGradientBoostingRegressor

[`QuantileRegressor`](https://scikit-learn.org/dev/modules/generated/sklearn.linear_model.QuantileRegressor.html#sklearn.linear_model.QuantileRegressor) and [`HistGradientBoostingRegressor`](https://scikit-learn.org/dev/modules/generated/sklearn.ensemble.HistGradientBoostingRegressor.html#sklearn.ensemble.HistGradientBoostingRegressor) both have a `quantile` parameter. But the meaning is the **probability level** of the quantile to be estimated/predicted, not the quantile itself.

R's [`quantile`](https://stat.ethz.ch/R-manual/R-devel/library/stats/html/quantile.html) function calls this parameter `probs` for probability.

Can we consider to rename it? Possible suggestions:
- `quantile_level`
- `probability`

COMMENT:
> I don't like probability. Maybe quantile_level is fine.

It really is a probability.

@Kaustbh This is not yet decided, see the label "Need Decision".

Labels: API, Needs Decision, module:ensemble, module:linear_model
RFC/API (Array API) mixing devices and data types with estimators

Right now, if the user fits an estimator using a `pandas.DataFrame`, but passes a `numpy.ndarray` during `predict`, they get a warning due to missing feature names.

The situation is only to get more complicated as we're adding support for more types via array API.

Some related issues here are:
- device: data during `fit` sits on a GPU, but a CPU is used for predict (with the same data type)
- types: using one type to `fit`, and use another type during `predict`: how do we handle this both in terms of device and the type? Do we let the operator figure out if they can coerce the data into the type which can be used?
- persistence: how do we let users fit on one device, but load on another device
- estimator conversion: do we let users convert an estimator which is fit using one type/device, to an estimator compatible with another type/device?

I vaguely remember us talking about some of these issues, but I don't see any active discussion. I might have missed something.

Related: in a library like `pytorch`, you can decide which device is going to be used when you load a model's weights.

cc @thomasjpfan

COMMENT:
Thinking about this a bit more I think an estimator should raise an explicit exception when it was fitted with, say, a torch tensor and the user is now trying to predict with a cupy array.

Right now, if you `fit` with torch and then try to `transform` a numpy array (with array API dispatch on or off) you get the following exception: `TypeError: unsupported operand type(s) for -: 'numpy.ndarray' and 'Tensor'`. Which is technically correct but I suspect pretty useless for the unsuspecting user.

For example a fitted estimator could check the array namespace of its fitted attributes and compare them to that of the input `X`.

Labels: API, RFC, Array API
RFC/API (Array API) mixing devices and data types with estimators

Right now, if the user fits an estimator using a `pandas.DataFrame`, but passes a `numpy.ndarray` during `predict`, they get a warning due to missing feature names.

The situation is only to get more complicated as we're adding support for more types via array API.

Some related issues here are:
- device: data during `fit` sits on a GPU, but a CPU is used for predict (with the same data type)
- types: using one type to `fit`, and use another type during `predict`: how do we handle this both in terms of device and the type? Do we let the operator figure out if they can coerce the data into the type which can be used?
- persistence: how do we let users fit on one device, but load on another device
- estimator conversion: do we let users convert an estimator which is fit using one type/device, to an estimator compatible with another type/device?

I vaguely remember us talking about some of these issues, but I don't see any active discussion. I might have missed something.

Related: in a library like `pytorch`, you can decide which device is going to be used when you load a model's weights.

cc @thomasjpfan

COMMENT:
> I vaguely remember us talking about some of these issues, but I don't see any active discussion. I might have missed something.

We discussed a bit about it when designing a generic tool to convert everything to numpy, namely `sklearn.utils._array_api._estimator_with_converted_arrays` as documented in:

- https://scikit-learn.org/dev/modules/array_api.html

> Thinking about this a bit more I think an estimator should raise an explicit exception when it was fitted with, say, a torch tensor and the user is now trying to predict with a cupy array.

I think it's safe to do so. If we change our minds in the future and decide to implement some kind of automated conversion, we can always do so without breaking user code while it's not possible to do it the other way around.

Labels: API, RFC, Array API