Talip7/scikit-learn-issues-multilabel huggingface dataset:
improve performance of cdist/pdist pairwise_distance for boolean matrices

### Describe the workflow you want to enable

If we have a boolean matrix, we can pack bits, use XOR and then `np.bitwise_count` (in numpy 2.0) to compute distances much faster than current `cdist` or `pdist` or `pairwise_distance`. It would be awesome to accelerate these functions.

### Describe your proposed solution

import numpy as np
import time
from sklearn.metrics import pairwise_distances
from scipy.spatial.distance import pdist, cdist 

qq1 = (np.random.rand(230099, 4097) > 0.5)
mm1 = (np.random.rand(810, 4097) > 0.5)

t1 = time.time()
d2a = cdist(qq1, mm1, metric='sqeuclidean')
print(f"{time.time() - t1}")
# returns 186.7363

t1 = time.time()
d2aa = cdist(qq1, mm1, metric='cityblock')
print(f"{time.time() - t1}")
# returns 335s

def fast_binary_dist(X, Y):
    out = np.zeros([X.shape[0], Y.shape[0]])
    for j in range(Y.shape[0]):
        row = Y.take(j, axis=0)
        out[:, j] = np.sum(np.bitwise_count(X ^ row), axis=1, dtype=int)
    return out

t1 = time.time()
qq1p = np.packbits(qq1, axis=1)
mm1p = np.packbits(mm1, axis=1)
d2c = fast_binary_dist(qq1p, mm1p)
print(f"{time.time() - t1}")
# returns 43.1 seconds

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
``metric="sqeuclidean"`` directly uses ``cdist(metric="sqeuclidean")`` from scipy while ``metric="euclidean"`` uses scikit-learn's own implementation.

Our implementation leverages the fact that ``||x-y||^2 = ||x||^2 + ||y||^2 - 2 <x,y>``, and thus can use a matrix-matrix multiplication for the critical part of the computation, which is done by a BLAS library (mkl, openblas, ...) and is very well optimized and parallelized.

The downside is that a bit of precision loss may occur, especially for distances between very close vectors.

Labels: Performance
improve performance of cdist/pdist pairwise_distance for boolean matrices

### Describe the workflow you want to enable

If we have a boolean matrix, we can pack bits, use XOR and then `np.bitwise_count` (in numpy 2.0) to compute distances much faster than current `cdist` or `pdist` or `pairwise_distance`. It would be awesome to accelerate these functions.

### Describe your proposed solution

import numpy as np
import time
from sklearn.metrics import pairwise_distances
from scipy.spatial.distance import pdist, cdist 

qq1 = (np.random.rand(230099, 4097) > 0.5)
mm1 = (np.random.rand(810, 4097) > 0.5)

t1 = time.time()
d2a = cdist(qq1, mm1, metric='sqeuclidean')
print(f"{time.time() - t1}")
# returns 186.7363

t1 = time.time()
d2aa = cdist(qq1, mm1, metric='cityblock')
print(f"{time.time() - t1}")
# returns 335s

def fast_binary_dist(X, Y):
    out = np.zeros([X.shape[0], Y.shape[0]])
    for j in range(Y.shape[0]):
        row = Y.take(j, axis=0)
        out[:, j] = np.sum(np.bitwise_count(X ^ row), axis=1, dtype=int)
    return out

t1 = time.time()
qq1p = np.packbits(qq1, axis=1)
mm1p = np.packbits(mm1, axis=1)
d2c = fast_binary_dist(qq1p, mm1p)
print(f"{time.time() - t1}")
# returns 43.1 seconds

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
This makes sense. It is a subtle difference that these two parallel implementations have performance/accuracy tradeoff variations. I wish we could make it more explicit somehow. But at this point consider this closed. Thank you very much for comments!!

Labels: Performance
Using `rng=` keyword argument  for NumPy randomness

In SPEC7 https://github.com/scientific-python/specs/pull/180, has two goals:

1. Deprecate the use of `RandomState` and `np.random.seed`
2. Standardize the usage of `rng` for setting seeding.

For 1, according to [NEP19](https://numpy.org/neps/nep-0019-rng-policy.html#numpy-random), I do not think NumPy wants to deprecate `np.random.seed` because they see valid use cases.

For 2, the primary reason around using `rng` instead of `random_state` is that it is a "better name" for NumPy's [Random Generator](https://numpy.org/doc/stable/reference/random/generator.html#random-generator). I am okay with keeping `random_state` and not have users go the pain of changing their code.

Currently, scikit-learn does not support generators because we tied it to https://github.com/scikit-learn/enhancement_proposals/pull/88. We wanted to use generators to cleanly switch to a different RNG behavior compared to RandomState. For me, I think they can be decoupled. If we tackle  https://github.com/scikit-learn/enhancement_proposals/pull/88, we can fix it for both RandomState and Generators.

@scikit-learn/core-devs What do you think of SPEC7's proposal?

COMMENT:
> For 2, the primary reason around using rng instead of random_state is that it is a "better name" for NumPy's [3]Random Generator. I am okay with keeping random_state and not have users go the pain of changing their code.

+1

It was interesting to me to witness the reactions to numpy 2.0. Senior and visible people who had been vocal to criticize historical choices in numpy and slowness of numpy to move forward where also the same to complain about breakage across their stacks and environments.

Labels: API, Needs Decision, RFC
Using `rng=` keyword argument  for NumPy randomness

In SPEC7 https://github.com/scientific-python/specs/pull/180, has two goals:

1. Deprecate the use of `RandomState` and `np.random.seed`
2. Standardize the usage of `rng` for setting seeding.

For 1, according to [NEP19](https://numpy.org/neps/nep-0019-rng-policy.html#numpy-random), I do not think NumPy wants to deprecate `np.random.seed` because they see valid use cases.

For 2, the primary reason around using `rng` instead of `random_state` is that it is a "better name" for NumPy's [Random Generator](https://numpy.org/doc/stable/reference/random/generator.html#random-generator). I am okay with keeping `random_state` and not have users go the pain of changing their code.

Currently, scikit-learn does not support generators because we tied it to https://github.com/scikit-learn/enhancement_proposals/pull/88. We wanted to use generators to cleanly switch to a different RNG behavior compared to RandomState. For me, I think they can be decoupled. If we tackle  https://github.com/scikit-learn/enhancement_proposals/pull/88, we can fix it for both RandomState and Generators.

@scikit-learn/core-devs What do you think of SPEC7's proposal?

COMMENT:
> It was interesting to me to witness the reactions to numpy 2.0. Senior and visible people who had been vocal to criticize historical choices in numpy and slowness of numpy to move forward where also the same to complain about breakage across their stacks and environments.

Oh well, I guess it's not that easy to make people happy :stuck_out_tongue_closed_eyes:

Labels: API, Needs Decision, RFC
Using `rng=` keyword argument  for NumPy randomness

In SPEC7 https://github.com/scientific-python/specs/pull/180, has two goals:

1. Deprecate the use of `RandomState` and `np.random.seed`
2. Standardize the usage of `rng` for setting seeding.

For 1, according to [NEP19](https://numpy.org/neps/nep-0019-rng-policy.html#numpy-random), I do not think NumPy wants to deprecate `np.random.seed` because they see valid use cases.

For 2, the primary reason around using `rng` instead of `random_state` is that it is a "better name" for NumPy's [Random Generator](https://numpy.org/doc/stable/reference/random/generator.html#random-generator). I am okay with keeping `random_state` and not have users go the pain of changing their code.

Currently, scikit-learn does not support generators because we tied it to https://github.com/scikit-learn/enhancement_proposals/pull/88. We wanted to use generators to cleanly switch to a different RNG behavior compared to RandomState. For me, I think they can be decoupled. If we tackle  https://github.com/scikit-learn/enhancement_proposals/pull/88, we can fix it for both RandomState and Generators.

@scikit-learn/core-devs What do you think of SPEC7's proposal?

COMMENT:
The `random_state` discussion got quite convoluted, and the last meeting that we talked about it a while ago, I didn't get the impression that we have a way forward.

I personally don't mind renaming `random_state` to `rng` IF it unlocks some backward compatibility concerns with _fixing_ `random_state`. New name, new behavior, otherwise I don't care much about changing the name because of the SPEC.

I also wouldn't deprecate `RandomState`'s usage, but happy to move all our own code / examples to the generators instead.

Labels: API, Needs Decision, RFC
Using `rng=` keyword argument  for NumPy randomness

In SPEC7 https://github.com/scientific-python/specs/pull/180, has two goals:

1. Deprecate the use of `RandomState` and `np.random.seed`
2. Standardize the usage of `rng` for setting seeding.

For 1, according to [NEP19](https://numpy.org/neps/nep-0019-rng-policy.html#numpy-random), I do not think NumPy wants to deprecate `np.random.seed` because they see valid use cases.

For 2, the primary reason around using `rng` instead of `random_state` is that it is a "better name" for NumPy's [Random Generator](https://numpy.org/doc/stable/reference/random/generator.html#random-generator). I am okay with keeping `random_state` and not have users go the pain of changing their code.

Currently, scikit-learn does not support generators because we tied it to https://github.com/scikit-learn/enhancement_proposals/pull/88. We wanted to use generators to cleanly switch to a different RNG behavior compared to RandomState. For me, I think they can be decoupled. If we tackle  https://github.com/scikit-learn/enhancement_proposals/pull/88, we can fix it for both RandomState and Generators.

@scikit-learn/core-devs What do you think of SPEC7's proposal?

COMMENT:
I agree that we should provide a benefit to the user if we make them change from `random_state` to `rng`. I don't know what that benefit would be though. Especially given that Numpy seems to have no plan to remove the old random number infrastructure. Basically, I am uneducated about the advantages and not aware of one but dismissive of it.

I like the idea of using the switch from `random_state=` to `rng=` to also fix the [randomness ambiguities](https://github.com/scikit-learn/enhancement_proposals/pull/88). However I also think that fixing both at the same time has a lower chance of happening than if we tackle them independently. The price to tackle them independently is more user upheaval, I think.



> I am okay with keeping `random_state` and not have users go the pain of changing their code.

Thomas, is this a proposal to add `rng=` without removing/deprecating `random_state=`?

Labels: API, Needs Decision, RFC
Using `rng=` keyword argument  for NumPy randomness

In SPEC7 https://github.com/scientific-python/specs/pull/180, has two goals:

1. Deprecate the use of `RandomState` and `np.random.seed`
2. Standardize the usage of `rng` for setting seeding.

For 1, according to [NEP19](https://numpy.org/neps/nep-0019-rng-policy.html#numpy-random), I do not think NumPy wants to deprecate `np.random.seed` because they see valid use cases.

For 2, the primary reason around using `rng` instead of `random_state` is that it is a "better name" for NumPy's [Random Generator](https://numpy.org/doc/stable/reference/random/generator.html#random-generator). I am okay with keeping `random_state` and not have users go the pain of changing their code.

Currently, scikit-learn does not support generators because we tied it to https://github.com/scikit-learn/enhancement_proposals/pull/88. We wanted to use generators to cleanly switch to a different RNG behavior compared to RandomState. For me, I think they can be decoupled. If we tackle  https://github.com/scikit-learn/enhancement_proposals/pull/88, we can fix it for both RandomState and Generators.

@scikit-learn/core-devs What do you think of SPEC7's proposal?

COMMENT:
> Thomas, is this a proposal to add rng= without removing/deprecating random_state=?

No. That proposal is to not use `rng` at all and continue to use `random_state` even when we support generators.

Labels: API, Needs Decision, RFC
Using `rng=` keyword argument  for NumPy randomness

In SPEC7 https://github.com/scientific-python/specs/pull/180, has two goals:

1. Deprecate the use of `RandomState` and `np.random.seed`
2. Standardize the usage of `rng` for setting seeding.

For 1, according to [NEP19](https://numpy.org/neps/nep-0019-rng-policy.html#numpy-random), I do not think NumPy wants to deprecate `np.random.seed` because they see valid use cases.

For 2, the primary reason around using `rng` instead of `random_state` is that it is a "better name" for NumPy's [Random Generator](https://numpy.org/doc/stable/reference/random/generator.html#random-generator). I am okay with keeping `random_state` and not have users go the pain of changing their code.

Currently, scikit-learn does not support generators because we tied it to https://github.com/scikit-learn/enhancement_proposals/pull/88. We wanted to use generators to cleanly switch to a different RNG behavior compared to RandomState. For me, I think they can be decoupled. If we tackle  https://github.com/scikit-learn/enhancement_proposals/pull/88, we can fix it for both RandomState and Generators.

@scikit-learn/core-devs What do you think of SPEC7's proposal?

COMMENT:
This means we'd also accept a random generator as argument to `random_state`? What problem do we solve with that? One that I can think of is people who use generators and expect them to work across the eco-system will have a good experience.

That seems like a good enough reason to allow people to pass a generator as `random_state=`.

Labels: API, Needs Decision, RFC
Why use the similarity matrix to determine the clusters instead of the A+R matrix?

Hi @GaelVaroquaux  

Thanks for the great effort of putting together this implementation and maintaining it. 

In the Affinity Propagation paper by Frey and Dueck, the `E=A+R` matrix is the one used to determine the cluster to which point belongs to, why are you using the similarity matrix here instead? 

https://github.com/scikit-learn/scikit-learn/blob/2621573e60c295a435c62137c65ae787bf438e61/sklearn/cluster/_affinity_propagation.py#L149

Also, why are you "refining" the exemplars and the clusters?

https://github.com/scikit-learn/scikit-learn/blob/2621573e60c295a435c62137c65ae787bf438e61/sklearn/cluster/_affinity_propagation.py#L152

COMMENT:
I have no idea. This was all a long time ago

Labels: Needs Triage
Why use the similarity matrix to determine the clusters instead of the A+R matrix?

Hi @GaelVaroquaux  

Thanks for the great effort of putting together this implementation and maintaining it. 

In the Affinity Propagation paper by Frey and Dueck, the `E=A+R` matrix is the one used to determine the cluster to which point belongs to, why are you using the similarity matrix here instead? 

https://github.com/scikit-learn/scikit-learn/blob/2621573e60c295a435c62137c65ae787bf438e61/sklearn/cluster/_affinity_propagation.py#L149

Also, why are you "refining" the exemplars and the clusters?

https://github.com/scikit-learn/scikit-learn/blob/2621573e60c295a435c62137c65ae787bf438e61/sklearn/cluster/_affinity_propagation.py#L152

COMMENT:
Hi @IsmaelElsharkawi, if this is just a question regarding implement details I think it would better be a github discussion. But if you think there's a bug, could you provide a reproducible example that demonstrate the issue ?

Labels: Needs Triage
Why use the similarity matrix to determine the clusters instead of the A+R matrix?

Hi @GaelVaroquaux  

Thanks for the great effort of putting together this implementation and maintaining it. 

In the Affinity Propagation paper by Frey and Dueck, the `E=A+R` matrix is the one used to determine the cluster to which point belongs to, why are you using the similarity matrix here instead? 

https://github.com/scikit-learn/scikit-learn/blob/2621573e60c295a435c62137c65ae787bf438e61/sklearn/cluster/_affinity_propagation.py#L149

Also, why are you "refining" the exemplars and the clusters?

https://github.com/scikit-learn/scikit-learn/blob/2621573e60c295a435c62137c65ae787bf438e61/sklearn/cluster/_affinity_propagation.py#L152

COMMENT:
I'll move it to a github discussion.

Labels: Needs Triage
Remove uneeded SKLEARN_BUILD_PARALLEL environment variable

This is only actually used if we build with setuptools and most of our CI builds are with Meson, i.e. the only setuptools build has `BUILD_WITH_SETUPTOOLS: 'true'`

Here are the usages of SKLEARN_BUILD_PARALLEL, most should be removed some should be kept (at least the one in `setup.py` and maybe the `azure/install.sh` although it could be only in the case of `BUILD_WITH_SETUPTOOLS == "true"`)
```
❯ git grep -P SKLEARN_BUILD_PARALLEL
.github/workflows/wheels.yml:            SKLEARN_BUILD_PARALLEL=3
.github/workflows/wheels.yml:          SKLEARN_BUILD_PARALLEL: 3
build_tools/azure/install.sh:    export SKLEARN_BUILD_PARALLEL=3
build_tools/circle/build_doc.sh:export SKLEARN_BUILD_PARALLEL=3
build_tools/cirrus/arm_wheel.yml:                      SKLEARN_BUILD_PARALLEL=5
build_tools/cirrus/build_test_arm.sh:export SKLEARN_BUILD_PARALLEL=$(($N_CORES + 1))
setup.py:            parallel = os.environ.get("SKLEARN_BUILD_PARALLEL")
```

COMMENT:
@clin1234 since you expressed some interest about familiarizing yourself with the scikit-learn CI, tackling this issue may be a good way to start!

Labels: Build / CI, help wanted
Remove uneeded SKLEARN_BUILD_PARALLEL environment variable

This is only actually used if we build with setuptools and most of our CI builds are with Meson, i.e. the only setuptools build has `BUILD_WITH_SETUPTOOLS: 'true'`

Here are the usages of SKLEARN_BUILD_PARALLEL, most should be removed some should be kept (at least the one in `setup.py` and maybe the `azure/install.sh` although it could be only in the case of `BUILD_WITH_SETUPTOOLS == "true"`)
```
❯ git grep -P SKLEARN_BUILD_PARALLEL
.github/workflows/wheels.yml:            SKLEARN_BUILD_PARALLEL=3
.github/workflows/wheels.yml:          SKLEARN_BUILD_PARALLEL: 3
build_tools/azure/install.sh:    export SKLEARN_BUILD_PARALLEL=3
build_tools/circle/build_doc.sh:export SKLEARN_BUILD_PARALLEL=3
build_tools/cirrus/arm_wheel.yml:                      SKLEARN_BUILD_PARALLEL=5
build_tools/cirrus/build_test_arm.sh:export SKLEARN_BUILD_PARALLEL=$(($N_CORES + 1))
setup.py:            parallel = os.environ.get("SKLEARN_BUILD_PARALLEL")
```

COMMENT:
Q: about the function that gets the environment var:
```python
def finalize_options(self):
        build_ext.finalize_options(self)
        if self.parallel is None:
            # Do not override self.parallel if already defined by
            # command-line flag (--parallel or -j)

            parallel = os.environ.get("SKLEARN_BUILD_PARALLEL")
            if parallel:
                self.parallel = int(parallel)
        if self.parallel:
            print("setting parallel=%d " % self.parallel)
```

Where in the build process does setup.py get either command flag for parallel?

Labels: Build / CI, help wanted
Remove uneeded SKLEARN_BUILD_PARALLEL environment variable

This is only actually used if we build with setuptools and most of our CI builds are with Meson, i.e. the only setuptools build has `BUILD_WITH_SETUPTOOLS: 'true'`

Here are the usages of SKLEARN_BUILD_PARALLEL, most should be removed some should be kept (at least the one in `setup.py` and maybe the `azure/install.sh` although it could be only in the case of `BUILD_WITH_SETUPTOOLS == "true"`)
```
❯ git grep -P SKLEARN_BUILD_PARALLEL
.github/workflows/wheels.yml:            SKLEARN_BUILD_PARALLEL=3
.github/workflows/wheels.yml:          SKLEARN_BUILD_PARALLEL: 3
build_tools/azure/install.sh:    export SKLEARN_BUILD_PARALLEL=3
build_tools/circle/build_doc.sh:export SKLEARN_BUILD_PARALLEL=3
build_tools/cirrus/arm_wheel.yml:                      SKLEARN_BUILD_PARALLEL=5
build_tools/cirrus/build_test_arm.sh:export SKLEARN_BUILD_PARALLEL=$(($N_CORES + 1))
setup.py:            parallel = os.environ.get("SKLEARN_BUILD_PARALLEL")
```

COMMENT:
For now you should not touch `setup.py`. Not sure about what you want to know with your question but `git grep` is your friend to answer this kind of things, so `git grep 'setup\.py'` will tell you which files mention it.

The only place in the CI where `setup.py` is used is here I think:
https://github.com/scikit-learn/scikit-learn/blob/36104b5788293cb1050b2533d63b281658511fd2/build_tools/azure/install.sh#L131-L132

All the other builds are using Meson which is our main supported build backend since scikit-learn 1.5. Just FYI, at one point in the future, we will get rid of `setup.py` and setuptools and we will use only Meson.

Generally speaking about the scikit-learn CI organization:
- `build_tools` have some CI specific things + other things as well
- we use different CI providers for different reasons (for example CircleCI is for the doc because they allow to have HTML preview in PR) but what I would call the "main" CI, i.e. the one that runs in each PR and runs the scikit-learn test suite is Azure with the configuration in https://github.com/scikit-learn/scikit-learn/blob/main/azure-pipelines.yml

Labels: Build / CI, help wanted
Remove uneeded SKLEARN_BUILD_PARALLEL environment variable

This is only actually used if we build with setuptools and most of our CI builds are with Meson, i.e. the only setuptools build has `BUILD_WITH_SETUPTOOLS: 'true'`

Here are the usages of SKLEARN_BUILD_PARALLEL, most should be removed some should be kept (at least the one in `setup.py` and maybe the `azure/install.sh` although it could be only in the case of `BUILD_WITH_SETUPTOOLS == "true"`)
```
❯ git grep -P SKLEARN_BUILD_PARALLEL
.github/workflows/wheels.yml:            SKLEARN_BUILD_PARALLEL=3
.github/workflows/wheels.yml:          SKLEARN_BUILD_PARALLEL: 3
build_tools/azure/install.sh:    export SKLEARN_BUILD_PARALLEL=3
build_tools/circle/build_doc.sh:export SKLEARN_BUILD_PARALLEL=3
build_tools/cirrus/arm_wheel.yml:                      SKLEARN_BUILD_PARALLEL=5
build_tools/cirrus/build_test_arm.sh:export SKLEARN_BUILD_PARALLEL=$(($N_CORES + 1))
setup.py:            parallel = os.environ.get("SKLEARN_BUILD_PARALLEL")
```

COMMENT:
> For now you should not touch `setup.py`. Not sure about what you want to know with your question but `git grep` is your friend to answer this kind of things, so `git grep 'setup\.py'` will tell you which files mention it.
> 
> The only place in the CI where `setup.py` is used is here I think:
> 
> https://github.com/scikit-learn/scikit-learn/blob/36104b5788293cb1050b2533d63b281658511fd2/build_tools/azure/install.sh#L131-L132
> 
> All the other builds are using Meson which is our main supported build backend since scikit-learn 1.5. Just FYI, at one point in the future, we will get rid of `setup.py` and setuptools and we will use only Meson.
> 
> Generally speaking about the scikit-learn CI organization:
> 
> * `build_tools` have some CI specific things + other things as well
> * we use different CI providers for different reasons (for example CircleCI is for the doc because they allow to have HTML preview in PR) but what I would call the "main" CI, i.e. the one that runs in each PR and runs the scikit-learn test suite is Azure with the configuration in https://github.com/scikit-learn/scikit-learn/blob/main/azure-pipelines.yml

Noob question, but does meson support parallel builds by default?

Labels: Build / CI, help wanted