Talip7/scikit-learn-issues-multilabel huggingface dataset:
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
Great question @glevv - I'm not qualified to address it. I can say that purely speaking, if the support of one distribution doesn't match that of the other (i.e. one has zero where the other is non-zero), strictly speaking their KL divergence should be infinity.
This brings to mind another consideration - if the X matrix size is in the order of magnitude that of `1/epsilon` (which would be the case now if X has 10k samples and 1k features), adding ε to all of the cells might make the calculation for a distribution matrix (Sum of all entries == 1 ) way off. I think this is another justification for lowering the value of ε to the 10^-15 domain.

Labels: module:decomposition
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
It's just the matter of reweighing the probabilities

> from scipy.special import kl_div
> probs = np.asarray([0., 0., 0.9, 0.1] * 1000) / 1000
> probs_y = np.asarray([0.1, 0.1, 0.7, 0.1] * 1000) / 1000
> EPSILON = 1e-7
> print(kl_div(probs, probs_y).sum()) # straightforward
> print(kl_div(probs * (1-EPSILON) + EPSILON / probs.shape[0], probs_y).sum()) # reweighed 
> print(kl_div(np.where(probs < EPSILON, EPSILON, probs), probs_y).sum()) # proposed

> 0.22618298545281545
> 0.22618215902713518
> 0.224601434397019

Higher epsilon (like 1e-3) will give very high discrepancy between proposed and straightforward method, while reweighed still will be close enough (3rd digit after floating point). With lower epsilon (like 1e-15) all the methods will "converge" to the same value.

If we reverse values (kl_div(probs_y, probs) ) we will get
> inf
> 2.864440919575757
> 1.2056309559997935

And second method will give higher number (but not infinity) than 3rd with any epsilon, which is what is needed.

Labels: module:decomposition
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
Allright, so how do you suggest I change the pull request (one or a combination of the following)?
option 1: just adjusting `EPSILON = 1E-15`
option 2: change `WH_data[WH_data < EPSILON] = ΕPSILON` to `WH_data += ΕPSILON`
option 3: readjust inside each location `eps = EPSILON / WH_data.shape[0]`

Labels: module:decomposition
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
I think it's up to core team to decide how to move forward with this issue

Labels: module:decomposition
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
Should this one be closed?

Labels: module:decomposition
Open up `check_array` and `BaseEstimator._validate_data` to overriding `xp.asarray` with an additional callable parameter `asarray_fn`

### Describe the workflow you want to enable

Some people (including @betatim @ogrisel @jjerphan and I) have been devising a plugin system that would open up `sklearn` estimators to other external implementations, and in particular implementations with GPU backends - see https://github.com/scikit-learn/scikit-learn/issues/22438 .

Some of the plugins we're considering can materialize the data in memory with an array library that is compatible with the Array API - namely [CuPy](https://github.com/scikit-learn/scikit-learn/issues/22438) and [dpctl.tensor](https://intelpython.github.io/dpctl/latest/docfiles/dpctl/dpctl.tensor_pyapi.html#dpctl-tensor-pyapi).

One thing we've found is that internally those plugins can benefit from using directly [`BaseEstimator._validate_data`](https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/base.py#L453) and [`check_array`](https://github.com/scikit-learn/scikit-learn/blob/d5fcb20b39c9b283d9485a9dcb4b2d554422e269/sklearn/utils/validation.py#L629) from `scikit-learn` to do the data acceptation and preparation step.

### Describe your proposed solution

To enable this it would be nice to be able to pass a `asarray_fn` to `check_array` and `_validate_data`, that would be called instead of `xp.asarray` in `_asarray_with_order` . This would enable the plugin to convert directly the input data to an array that the plugin supports (e.g. `cupy` or `dpctl.tensor`) while still benefiting from reusing existing validation code in `check_array`. 

The override can be necessary in case the `asarray` method from the array library implements a superset of the array api that is necessary for the plugin, but is currently not used by `check_array` because it's not part of the array api (for instance, the `order` argument isn't passed to `asarray` for array libraries other than `numpy`)

COMMENT:
Example given in https://github.com/scikit-learn/scikit-learn/pull/25434

Labels: New Feature, RFC
Open up `check_array` and `BaseEstimator._validate_data` to overriding `xp.asarray` with an additional callable parameter `asarray_fn`

### Describe the workflow you want to enable

Some people (including @betatim @ogrisel @jjerphan and I) have been devising a plugin system that would open up `sklearn` estimators to other external implementations, and in particular implementations with GPU backends - see https://github.com/scikit-learn/scikit-learn/issues/22438 .

Some of the plugins we're considering can materialize the data in memory with an array library that is compatible with the Array API - namely [CuPy](https://github.com/scikit-learn/scikit-learn/issues/22438) and [dpctl.tensor](https://intelpython.github.io/dpctl/latest/docfiles/dpctl/dpctl.tensor_pyapi.html#dpctl-tensor-pyapi).

One thing we've found is that internally those plugins can benefit from using directly [`BaseEstimator._validate_data`](https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/base.py#L453) and [`check_array`](https://github.com/scikit-learn/scikit-learn/blob/d5fcb20b39c9b283d9485a9dcb4b2d554422e269/sklearn/utils/validation.py#L629) from `scikit-learn` to do the data acceptation and preparation step.

### Describe your proposed solution

To enable this it would be nice to be able to pass a `asarray_fn` to `check_array` and `_validate_data`, that would be called instead of `xp.asarray` in `_asarray_with_order` . This would enable the plugin to convert directly the input data to an array that the plugin supports (e.g. `cupy` or `dpctl.tensor`) while still benefiting from reusing existing validation code in `check_array`. 

The override can be necessary in case the `asarray` method from the array library implements a superset of the array api that is necessary for the plugin, but is currently not used by `check_array` because it's not part of the array api (for instance, the `order` argument isn't passed to `asarray` for array libraries other than `numpy`)

COMMENT:
It was discussed at the triage meeting:

- people are concerned with kwarg proliferation and the added maintenance burden but we don't see any alternative to avoid duplicating the code of `check_array` otherwise;
- it was suggested to add this kwarg in a private version of `_check_array` while preserving the public `check_array` parameter list unchanged.

Alternatively, we could use the `sklearn.experimental` explicit activation of this feature.

Labels: New Feature, RFC
Open up `check_array` and `BaseEstimator._validate_data` to overriding `xp.asarray` with an additional callable parameter `asarray_fn`

### Describe the workflow you want to enable

Some people (including @betatim @ogrisel @jjerphan and I) have been devising a plugin system that would open up `sklearn` estimators to other external implementations, and in particular implementations with GPU backends - see https://github.com/scikit-learn/scikit-learn/issues/22438 .

Some of the plugins we're considering can materialize the data in memory with an array library that is compatible with the Array API - namely [CuPy](https://github.com/scikit-learn/scikit-learn/issues/22438) and [dpctl.tensor](https://intelpython.github.io/dpctl/latest/docfiles/dpctl/dpctl.tensor_pyapi.html#dpctl-tensor-pyapi).

One thing we've found is that internally those plugins can benefit from using directly [`BaseEstimator._validate_data`](https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/base.py#L453) and [`check_array`](https://github.com/scikit-learn/scikit-learn/blob/d5fcb20b39c9b283d9485a9dcb4b2d554422e269/sklearn/utils/validation.py#L629) from `scikit-learn` to do the data acceptation and preparation step.

### Describe your proposed solution

To enable this it would be nice to be able to pass a `asarray_fn` to `check_array` and `_validate_data`, that would be called instead of `xp.asarray` in `_asarray_with_order` . This would enable the plugin to convert directly the input data to an array that the plugin supports (e.g. `cupy` or `dpctl.tensor`) while still benefiting from reusing existing validation code in `check_array`. 

The override can be necessary in case the `asarray` method from the array library implements a superset of the array api that is necessary for the plugin, but is currently not used by `check_array` because it's not part of the array api (for instance, the `order` argument isn't passed to `asarray` for array libraries other than `numpy`)

COMMENT:
After working on Array API a little more, I propose adding a namespace `xp` parameter to `check_array`. For array libraries that do not define `__array_namespace__`, one can pass a namespace. The namespace does not have to be full compatible with the Array API specification. It only needs the functions required by `check_array`. Afterwards, we adjust `_asarray_with_order` to inspect the signature of `asarray` and if it accepts `order`, then pass it in.

This proposal should enable the Engine/Plugin API to use the Array API code paths for validation in `check_array`.

Labels: New Feature, RFC
Open up `check_array` and `BaseEstimator._validate_data` to overriding `xp.asarray` with an additional callable parameter `asarray_fn`

### Describe the workflow you want to enable

Some people (including @betatim @ogrisel @jjerphan and I) have been devising a plugin system that would open up `sklearn` estimators to other external implementations, and in particular implementations with GPU backends - see https://github.com/scikit-learn/scikit-learn/issues/22438 .

Some of the plugins we're considering can materialize the data in memory with an array library that is compatible with the Array API - namely [CuPy](https://github.com/scikit-learn/scikit-learn/issues/22438) and [dpctl.tensor](https://intelpython.github.io/dpctl/latest/docfiles/dpctl/dpctl.tensor_pyapi.html#dpctl-tensor-pyapi).

One thing we've found is that internally those plugins can benefit from using directly [`BaseEstimator._validate_data`](https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/base.py#L453) and [`check_array`](https://github.com/scikit-learn/scikit-learn/blob/d5fcb20b39c9b283d9485a9dcb4b2d554422e269/sklearn/utils/validation.py#L629) from `scikit-learn` to do the data acceptation and preparation step.

### Describe your proposed solution

To enable this it would be nice to be able to pass a `asarray_fn` to `check_array` and `_validate_data`, that would be called instead of `xp.asarray` in `_asarray_with_order` . This would enable the plugin to convert directly the input data to an array that the plugin supports (e.g. `cupy` or `dpctl.tensor`) while still benefiting from reusing existing validation code in `check_array`. 

The override can be necessary in case the `asarray` method from the array library implements a superset of the array api that is necessary for the plugin, but is currently not used by `check_array` because it's not part of the array api (for instance, the `order` argument isn't passed to `asarray` for array libraries other than `numpy`)

COMMENT:
That sounds great. it would answer the usecases I have reported with `dpctl`.

Labels: New Feature, RFC
⚠️ CI failed on Linux_nogil.pylatest_pip_nogil ⚠️

**CI is still failing on [Linux_nogil.pylatest_pip_nogil](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=51160&view=logs&j=67fbb25f-e417-50be-be55-3b1e9637fce5)** (Jan 20, 2023)
Unable to find junit file. Please see link for details.

COMMENT:
It seems that for this build, `python-nogil` pick up the version of Cython that it likes. I assume that we cannot do anything up to the point `python-nogil` request the latest Cython version?

Is that correct @lesteve?

Labels: Needs Triage
⚠️ CI failed on Linux_nogil.pylatest_pip_nogil ⚠️

**CI is still failing on [Linux_nogil.pylatest_pip_nogil](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=51160&view=logs&j=67fbb25f-e417-50be-be55-3b1e9637fce5)** (Jan 20, 2023)
Unable to find junit file. Please see link for details.

COMMENT:
It seems like Cython 0.29.33 is not available for nogil yet, only Cython 0.29.32.

There is also a pip requirements and associated lock file for nogil but it has to be updated manually: 
https://github.com/scikit-learn/scikit-learn/blob/main/build_tools/azure/python_nogil_requirements.txt

Labels: Needs Triage
⚠️ CI failed on Linux_nogil.pylatest_pip_nogil ⚠️

**CI is still failing on [Linux_nogil.pylatest_pip_nogil](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=51160&view=logs&j=67fbb25f-e417-50be-be55-3b1e9637fce5)** (Jan 20, 2023)
Unable to find junit file. Please see link for details.

COMMENT:
OK, so we can just wait. The build will fail early so this is not that bad.

Labels: Needs Triage
⚠️ CI failed on Linux_nogil.pylatest_pip_nogil ⚠️

**CI is still failing on [Linux_nogil.pylatest_pip_nogil](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=51160&view=logs&j=67fbb25f-e417-50be-be55-3b1e9637fce5)** (Jan 20, 2023)
Unable to find junit file. Please see link for details.

COMMENT:
Yep, I am not sure this is the right workflow (probably not exactly the right one ...) but I opened https://github.com/colesbury/nogil-wheels/pull/3 to show scikit-learn interest in cython 0.29.33.

Labels: Needs Triage
There is no raise expectation

### Describe the issue linked to the documentation

There is no raise expectation in baysius model when the prediction's feature is not in the data trained, but just index out of range. But there's raise value error in fitting function
<img width="303" alt="image" src="https://user-images.githubusercontent.com/49092631/212972056-5f04d2f8-b73d-43fb-a976-0ee65d139d44.png">


### Suggest a potential alternative/fix

add raise valueerror when the inputted prediction's feature is not trained (input) in the fittings
this took me three hours to find that bug

COMMENT:
<img width="508" alt="image" src="https://user-images.githubusercontent.com/49092631/212972548-be4c53dd-4ec0-4f46-98f1-ccdd6ecf90b1.png">

Labels: Documentation, Needs Reproducible Code
There is no raise expectation

### Describe the issue linked to the documentation

There is no raise expectation in baysius model when the prediction's feature is not in the data trained, but just index out of range. But there's raise value error in fitting function
<img width="303" alt="image" src="https://user-images.githubusercontent.com/49092631/212972056-5f04d2f8-b73d-43fb-a976-0ee65d139d44.png">


### Suggest a potential alternative/fix

add raise valueerror when the inputted prediction's feature is not trained (input) in the fittings
this took me three hours to find that bug

COMMENT:
Please provide [self-contained example code](https://stackoverflow.com/help/mcve), including imports and data (if possible), so that other contributors can just run it and reproduce your issue. Ideally your example code should be minimal.

Labels: Documentation, Needs Reproducible Code
There is no raise expectation

### Describe the issue linked to the documentation

There is no raise expectation in baysius model when the prediction's feature is not in the data trained, but just index out of range. But there's raise value error in fitting function
<img width="303" alt="image" src="https://user-images.githubusercontent.com/49092631/212972056-5f04d2f8-b73d-43fb-a976-0ee65d139d44.png">


### Suggest a potential alternative/fix

add raise valueerror when the inputted prediction's feature is not trained (input) in the fittings
this took me three hours to find that bug

COMMENT:
Can I work on this issue

Labels: Documentation, Needs Reproducible Code
There is no raise expectation

### Describe the issue linked to the documentation

There is no raise expectation in baysius model when the prediction's feature is not in the data trained, but just index out of range. But there's raise value error in fitting function
<img width="303" alt="image" src="https://user-images.githubusercontent.com/49092631/212972056-5f04d2f8-b73d-43fb-a976-0ee65d139d44.png">


### Suggest a potential alternative/fix

add raise valueerror when the inputted prediction's feature is not trained (input) in the fittings
this took me three hours to find that bug

COMMENT:
no, i mean it just need a raise error that reminds if the input test data is not included in the training data provided

Labels: Documentation, Needs Reproducible Code
Add Warnings when trying to use estimator.fit()  functions with Pandas objects with different indices.

### Describe the workflow you want to enable

Because sklearn uses numpy.array objects but accepts `pandas.DataFrame` / `pandas.Series` objects as input to its many functions, passing two pandas objects with different indices to a `estimator.fit()` function allows to unadvertedly work on shuffled X and y arrays. 

Let's construct an `X` `pandas.DataFrame` and an `y` `pandas.Series`.

```
import pandas as pd

x1 = [3, 3, 4, 5, 3, 5]
x2 = [0.1, 0.2, -2, -3, -3, -2]

y = ['a', 'a', 'b', 'b', 'b', 'b']

X_index = [1, 2, 3, 4, 5, 6]
y_index = [6, 5, 4, 3, 2, 1]

X = pd.DataFrame({'A': x1, 'B': x2}, index=X_index)
y = pd.Series(y, index=y_index)
```
Different pre-processing protocols (such as the use of `pandas.DataFrame.merge()`) can unadvertedly change the index of one of the two `pandas` objects (usually `X`, which is normally subject to more modifications than `y`).

Passing two incompatible indices like the ones above to an `estimator.fit()` function will yield no kind of warning. The model will be fit on two data objects which can potentially be in different orders, without expliciting the fact that the two indices are not equal. 

```
from sklearn.linear_model import LogisticRegression

model = LogisticRegression()
model.fit(X, y)
>>> LogisticRegression()

X.index == y.index
>>> array([False, False, False, False, False, False])
```

Checking if the two `pandas` objects' indices are equal and warning if they are not would allow users to understand that their `X` and `y` inputs may not be compatible. This can potentially save _a lot_ of time spent modeling on randomly shuffled data without warnings of any kind. Git knows I would've saved a lot of time. 

### Describe your proposed solution

For all fitting functions, implementing a simple check that  `X.index == y.index` is true for all cases before turning the `pandas` objects to `numpy`, and printing a warning informing that the indices are not compatible if it is not the case, urging the user to check that the order of their data is correct.

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

_No response_

### Additional context

_No response_

COMMENT:
The reason that we don't have such a warning is that pandas objects are not scikit-learn first class citizen has are the NumPy array or scipy sparse matrices.

If we are still fine with this contract, I will not make any guarantee on pandas object and let the responsibilities to the user to provide row-aligned data.

Labels: New Feature, Needs Decision