Talip7/scikit-learn-issues-multilabel huggingface dataset:
MAINT, RFC Simplify the Cython code in `sklearn/tree/` by splitting the "Splitter" and "Partitioner" code

### Summary of the problem
There are quite a number of GH issues with the label `tree` (https://github.com/scikit-learn/scikit-learn/issues?page=2&q=is%3Aopen+is%3Aissue+label%3Amodule%3Atree).

However, the code is a bit hard to approach as there's so many moving parts. For example, see: https://github.com/scikit-learn/scikit-learn/issues/18448#issuecomment-702598316, where users are intimidated by the Cython codebase. A large part of the complexity comes in the splitter, which contains the most algorithmic logic. I think with the work done by @thomasjpfan recently, we were able to pull apart the idea of a "partitioner" and "splitter" in the trees. 

This problem also arises because https://github.com/scikit-learn/scikit-learn/pull/29437/files#diff-e2cca285e1e883ab1d427120dfa974c1ba83eb6e2f5d5f416bbd99717ca5f5fc makes the code-diff very large inside `_splitter.pyx` file making it also harder to review and understand what changes affect the "splitter" and what changes affect the "partitioner".

### Proposed change
I further propose to split these into separate files, so it is easier to maintain, read and determine what is affecting what. In addition, we can decrease the code complexity by creating an abstract base class for the `DensePartitioner` and `SparsePartitioner`.

See the following [PR](https://github.com/scikit-learn/scikit-learn/pull/29458) for an example of what would change. The `_splitter.pyx` file would decrease by over 800 LOC, while it gets moved to `_partitioner.pyx`.

COMMENT:
I think that I'm fine with the split. One thing that we need to do when splitting the file is to exactly explain the job of the splitter and partitioner: looking at the name one could think that they do the same thing (they cut data).

I therefore think that we need to have a kind of developer documentation on the top of the file to explain why those are split: as far I remember, we have the partitioner to avoid speed regression due to some overhead in class inheritance.

> we can decrease the code complexity by creating an abstract base class for the DensePartitioner and SparsePartitioner

I think this something that would lead to regression and the current workaround is actually a workaround abstract class.

@thomasjpfan would know the details. Having his feedback here would be wonderful because we could use it to document the files.

Labels: module:tree, cython
MAINT, RFC Simplify the Cython code in `sklearn/tree/` by splitting the "Splitter" and "Partitioner" code

### Summary of the problem
There are quite a number of GH issues with the label `tree` (https://github.com/scikit-learn/scikit-learn/issues?page=2&q=is%3Aopen+is%3Aissue+label%3Amodule%3Atree).

However, the code is a bit hard to approach as there's so many moving parts. For example, see: https://github.com/scikit-learn/scikit-learn/issues/18448#issuecomment-702598316, where users are intimidated by the Cython codebase. A large part of the complexity comes in the splitter, which contains the most algorithmic logic. I think with the work done by @thomasjpfan recently, we were able to pull apart the idea of a "partitioner" and "splitter" in the trees. 

This problem also arises because https://github.com/scikit-learn/scikit-learn/pull/29437/files#diff-e2cca285e1e883ab1d427120dfa974c1ba83eb6e2f5d5f416bbd99717ca5f5fc makes the code-diff very large inside `_splitter.pyx` file making it also harder to review and understand what changes affect the "splitter" and what changes affect the "partitioner".

### Proposed change
I further propose to split these into separate files, so it is easier to maintain, read and determine what is affecting what. In addition, we can decrease the code complexity by creating an abstract base class for the `DensePartitioner` and `SparsePartitioner`.

See the following [PR](https://github.com/scikit-learn/scikit-learn/pull/29458) for an example of what would change. The `_splitter.pyx` file would decrease by over 800 LOC, while it gets moved to `_partitioner.pyx`.

COMMENT:
Here is my current understanding of Partitioner and Splitter (I can edit this as my understanding improves):

**Splitter:** handles the algorithm for determining a split node, either via a greedy, or random search over a `max_features` set of chosen features. This is sort of the "director" for how partitioner and criterion work together to determine, evaluate and apply the split. This is invariant to whether or not the data is sparse, or dense, as the partitioner handles how to actually separate the samples.

**Partitioner:** this performs the actual partitioning of the samples index array based on split points, missing values, etc. In the future is where we would partition samples to left/right as well for categorical splits.

Labels: module:tree, cython
MAINT, RFC Simplify the Cython code in `sklearn/tree/` by splitting the "Splitter" and "Partitioner" code

### Summary of the problem
There are quite a number of GH issues with the label `tree` (https://github.com/scikit-learn/scikit-learn/issues?page=2&q=is%3Aopen+is%3Aissue+label%3Amodule%3Atree).

However, the code is a bit hard to approach as there's so many moving parts. For example, see: https://github.com/scikit-learn/scikit-learn/issues/18448#issuecomment-702598316, where users are intimidated by the Cython codebase. A large part of the complexity comes in the splitter, which contains the most algorithmic logic. I think with the work done by @thomasjpfan recently, we were able to pull apart the idea of a "partitioner" and "splitter" in the trees. 

This problem also arises because https://github.com/scikit-learn/scikit-learn/pull/29437/files#diff-e2cca285e1e883ab1d427120dfa974c1ba83eb6e2f5d5f416bbd99717ca5f5fc makes the code-diff very large inside `_splitter.pyx` file making it also harder to review and understand what changes affect the "splitter" and what changes affect the "partitioner".

### Proposed change
I further propose to split these into separate files, so it is easier to maintain, read and determine what is affecting what. In addition, we can decrease the code complexity by creating an abstract base class for the `DensePartitioner` and `SparsePartitioner`.

See the following [PR](https://github.com/scikit-learn/scikit-learn/pull/29458) for an example of what would change. The `_splitter.pyx` file would decrease by over 800 LOC, while it gets moved to `_partitioner.pyx`.

COMMENT:
I ran some benchmarks using asv on the [PR](https://github.com/scikit-learn/scikit-learn/pull/29458) branch and the one on `main`, and I actually don't see any real performance diffs:

`asv run --bench RandomForestClassifierBenchmark.time_fit —verbose`

```
Partition PR (fused type)
[100.00%] ··· ================ =========
              --                 n_jobs 
              ---------------- ---------
               representation      1    
              ================ =========
                   dense        5.34±0s 
                   sparse       6.62±0s 
              ================ =========

Main 
[100.00%] ··· ================ ============
              --                  n_jobs   
              ---------------- ------------
               representation       1      
              ================ ============
                   dense        5.32±0.02s 
                   sparse        6.63±0s   
              ================ ============
```

Perhaps @thomasjpfan has some insight into when the vtable lookup hit was occurring? Is it only if the abstract base class has methods actually defined rather than "pass", or something else perhaps.

Labels: module:tree, cython
MAINT, RFC Simplify the Cython code in `sklearn/tree/` by splitting the "Splitter" and "Partitioner" code

### Summary of the problem
There are quite a number of GH issues with the label `tree` (https://github.com/scikit-learn/scikit-learn/issues?page=2&q=is%3Aopen+is%3Aissue+label%3Amodule%3Atree).

However, the code is a bit hard to approach as there's so many moving parts. For example, see: https://github.com/scikit-learn/scikit-learn/issues/18448#issuecomment-702598316, where users are intimidated by the Cython codebase. A large part of the complexity comes in the splitter, which contains the most algorithmic logic. I think with the work done by @thomasjpfan recently, we were able to pull apart the idea of a "partitioner" and "splitter" in the trees. 

This problem also arises because https://github.com/scikit-learn/scikit-learn/pull/29437/files#diff-e2cca285e1e883ab1d427120dfa974c1ba83eb6e2f5d5f416bbd99717ca5f5fc makes the code-diff very large inside `_splitter.pyx` file making it also harder to review and understand what changes affect the "splitter" and what changes affect the "partitioner".

### Proposed change
I further propose to split these into separate files, so it is easier to maintain, read and determine what is affecting what. In addition, we can decrease the code complexity by creating an abstract base class for the `DensePartitioner` and `SparsePartitioner`.

See the following [PR](https://github.com/scikit-learn/scikit-learn/pull/29458) for an example of what would change. The `_splitter.pyx` file would decrease by over 800 LOC, while it gets moved to `_partitioner.pyx`.

COMMENT:
> Having his feedback here would be wonderful because we could use it to document the files.

The approach in https://github.com/scikit-learn/scikit-learn/pull/29458 should be okay when it comes to v-table lookups. I left a review in https://github.com/scikit-learn/scikit-learn/pull/29458#pullrequestreview-2178691654 with my thoughts. Overall, I do not really want to depend on Cython to have the correct v-table behavior with a base class. The alternative is to to define the partitioner interface with a comment rather than an actual base class.

Labels: module:tree, cython
MAINT Remove scipy<1.6 specific code in QuantileRegressor and example

We don't need this code anymore since our minimum supported version is scipy 1.6:

https://github.com/scikit-learn/scikit-learn/blob/fa14001fa19a262c7eb43b2ef3c0d6b56b4c8fad/examples/linear_model/plot_quantile_regression.py#L112-L116

While we are at it there is likely some clode that can be removed in `sklearn/linear_model/_quantile.py`, for example:

https://github.com/scikit-learn/scikit-learn/blob/fa14001fa19a262c7eb43b2ef3c0d6b56b4c8fad/sklearn/linear_model/_quantile.py#L50-L52

https://github.com/scikit-learn/scikit-learn/blob/fa14001fa19a262c7eb43b2ef3c0d6b56b4c8fad/sklearn/linear_model/_quantile.py#L183-L190

COMMENT:
Hi @lesteve, I am interested in working on this issue. I am new to this. Please give me some time to identify all the scipy<1.6 specific code in quantile_regressor.py file.

Labels: good first issue, help wanted
MAINT Remove scipy<1.6 specific code in QuantileRegressor and example

We don't need this code anymore since our minimum supported version is scipy 1.6:

https://github.com/scikit-learn/scikit-learn/blob/fa14001fa19a262c7eb43b2ef3c0d6b56b4c8fad/examples/linear_model/plot_quantile_regression.py#L112-L116

While we are at it there is likely some clode that can be removed in `sklearn/linear_model/_quantile.py`, for example:

https://github.com/scikit-learn/scikit-learn/blob/fa14001fa19a262c7eb43b2ef3c0d6b56b4c8fad/sklearn/linear_model/_quantile.py#L50-L52

https://github.com/scikit-learn/scikit-learn/blob/fa14001fa19a262c7eb43b2ef3c0d6b56b4c8fad/sklearn/linear_model/_quantile.py#L183-L190

COMMENT:
Hi @lesteve , Can you please review this PR - https://github.com/scikit-learn/scikit-learn/pull/29461 ?

Labels: good first issue, help wanted
Why 30 neighbors in 'Agglomerative clustering with and without structure'?

### Describe the issue linked to the documentation

This is not so much an issue as a request for explanation. I was going through the scikit-learn user guide on 'Agglomerative clustering with and without structure', which can be found at https://scikit-learn.org/stable/auto_examples/cluster/plot_agglomerative_clustering.html#sphx-glr-auto-examples-cluster-plot-agglomerative-clustering-py. I decided to try it out by myself and included scaling of the `X` matrix using `MinMaxScaler()`.

Here is where the modification occured:
```
# ...

# Generate sample data
n_samples = 1500
np.random.seed(0)
t = 1.5 * np.pi * (1 + 3 * np.random.rand(1, n_samples))
x = t * np.cos(t)
y = t * np.sin(t)


X = np.concatenate((x, y))
X += 0.7 * np.random.randn(2, n_samples)
X = X.T

# ADDITIONAL STEP: PERFORM SCALING
X = MinMaxScaler().fit_transform(X)

# ...
```

I was surprised by the result. The connectivity constraints seem to have no effect on `n_clusters=3, connectivity=True, linkage='ward'` case:
![fig](https://github.com/scikit-learn/scikit-learn/assets/143477305/ef5d530f-8fb4-41ff-8bd9-e39b2ac8f5ed)

I realized this might be because scaling causes the nodes to come closer together, and 30 neighbors taken in the `knn_graph` might include cross-layer nodes. Therefore, I reduced it to 20 neighbors:

```
# ...

# Create a graph capturing local connectivity. Larger number of neighbors
# will give more homogeneous clusters to the cost of computation
# time. A very large number of neighbors gives more evenly distributed
# cluster sizes, but may not impose the local manifold structure of
# the data
# EDIT: CHANGE n_neighbors FROM 30 TO 20
knn_graph = kneighbors_graph(X, 20, include_self=False)

# ...
```

And then, I got better results.
![fig2](https://github.com/scikit-learn/scikit-learn/assets/143477305/ad6b06f6-4194-4616-84ab-c3ff73e0ec69)

This led me to think about how 30 neighbors was decided for the docs, and if there was a way to analytically determine this for every dataset.

Any clarification to this end would be greatly appreciated!

### Suggest a potential alternative/fix

Kindly add a section to https://scikit-learn.org/stable/auto_examples/cluster/plot_agglomerative_clustering.html#agglomerative-clustering-with-and-without-structure explaining how to choose `n_neighbors` for the `kneighbors_graph`.

COMMENT:
Moving this issue in discussion since this is a better place to get feedback.

Labels: Documentation, Needs Triage
Suggesting updates on the doc of `sklearn.neighbors.NeighborhoodComponentsAnalysis`

### Describe the issue linked to the documentation

Hi,

We are an academic team of software engineering researchers from a university working on automated program analysis techniques to improve API documentation quality, ultimately contributing to improving data science software development practices. we would like to keep anonymity for the purpose of double-blind paper reviewing.

We discover an inconsistency issue between documentation and code in the class [`sklearn.neighbors.NeighborhoodComponentsAnalysis`](https://scikit-learn.org/dev/modules/generated/sklearn.neighbors.NeighborhoodComponentsAnalysis.html#neighborhoodcomponentsanalysis). As mentioned in the description of the value `auto` of parameter `init`:

> -  **'auto'**
Depending on `n_components`, the most reasonable initialization will be chosen. **If `n_components <= n_classes` we use `'lda'`**, as it uses labels information. If not, but `n_components < min(n_features, n_samples)`, we use `'pca'`, as it projects data in meaningful directions (those of higher variance). Otherwise, we just use `'identity'`.

However, the most relevant source code snippet looks like this:

```python
if init == "auto":
    n_classes = len(np.unique(y))
    if n_components <= min(n_features, n_classes - 1):
        init = "lda"
    elif n_components < min(n_features, n_samples):
        init = "pca"
    else:
        init = "identity"
```

Apparently, the condition (**_n_components <= n_classes_**) shown in the document are different from the condition (**_n_components <= min(n_features, n_classes - 1)_**) actually executed in the code.


### Suggest a potential alternative/fix

Maybe you can update the documentation to avoid unnecessary misunderstanding.

COMMENT:
Hi. Yeah, it seems that there is a misunderstanding in the docs. I can work on this!

Labels: Documentation, spam
Suggesting updates on the doc of `sklearn.neighbors.NeighborhoodComponentsAnalysis`

### Describe the issue linked to the documentation

Hi,

We are an academic team of software engineering researchers from a university working on automated program analysis techniques to improve API documentation quality, ultimately contributing to improving data science software development practices. we would like to keep anonymity for the purpose of double-blind paper reviewing.

We discover an inconsistency issue between documentation and code in the class [`sklearn.neighbors.NeighborhoodComponentsAnalysis`](https://scikit-learn.org/dev/modules/generated/sklearn.neighbors.NeighborhoodComponentsAnalysis.html#neighborhoodcomponentsanalysis). As mentioned in the description of the value `auto` of parameter `init`:

> -  **'auto'**
Depending on `n_components`, the most reasonable initialization will be chosen. **If `n_components <= n_classes` we use `'lda'`**, as it uses labels information. If not, but `n_components < min(n_features, n_samples)`, we use `'pca'`, as it projects data in meaningful directions (those of higher variance). Otherwise, we just use `'identity'`.

However, the most relevant source code snippet looks like this:

```python
if init == "auto":
    n_classes = len(np.unique(y))
    if n_components <= min(n_features, n_classes - 1):
        init = "lda"
    elif n_components < min(n_features, n_samples):
        init = "pca"
    else:
        init = "identity"
```

Apparently, the condition (**_n_components <= n_classes_**) shown in the document are different from the condition (**_n_components <= min(n_features, n_classes - 1)_**) actually executed in the code.


### Suggest a potential alternative/fix

Maybe you can update the documentation to avoid unnecessary misunderstanding.

COMMENT:
I just submitted a PR to fix this: #29441

Labels: Documentation, spam
Suggesting updates on the doc of `sklearn.neighbors.NeighborhoodComponentsAnalysis`

### Describe the issue linked to the documentation

Hi,

We are an academic team of software engineering researchers from a university working on automated program analysis techniques to improve API documentation quality, ultimately contributing to improving data science software development practices. we would like to keep anonymity for the purpose of double-blind paper reviewing.

We discover an inconsistency issue between documentation and code in the class [`sklearn.neighbors.NeighborhoodComponentsAnalysis`](https://scikit-learn.org/dev/modules/generated/sklearn.neighbors.NeighborhoodComponentsAnalysis.html#neighborhoodcomponentsanalysis). As mentioned in the description of the value `auto` of parameter `init`:

> -  **'auto'**
Depending on `n_components`, the most reasonable initialization will be chosen. **If `n_components <= n_classes` we use `'lda'`**, as it uses labels information. If not, but `n_components < min(n_features, n_samples)`, we use `'pca'`, as it projects data in meaningful directions (those of higher variance). Otherwise, we just use `'identity'`.

However, the most relevant source code snippet looks like this:

```python
if init == "auto":
    n_classes = len(np.unique(y))
    if n_components <= min(n_features, n_classes - 1):
        init = "lda"
    elif n_components < min(n_features, n_samples):
        init = "pca"
    else:
        init = "identity"
```

Apparently, the condition (**_n_components <= n_classes_**) shown in the document are different from the condition (**_n_components <= min(n_features, n_classes - 1)_**) actually executed in the code.


### Suggest a potential alternative/fix

Maybe you can update the documentation to avoid unnecessary misunderstanding.

COMMENT:
It seems there is also potentially a discrepancy in the description of `'lda'` https://github.com/scikit-learn/scikit-learn/blob/a6a5397bcb95c0653df449ea8ae982d6bc6566c0/sklearn/neighbors/_nca.py#L70-L75.

Unsure why there is a `n_classes - 1` there actually in the code.

Labels: Documentation, spam
Suggesting updates on the doc of `sklearn.neighbors.NeighborhoodComponentsAnalysis`

### Describe the issue linked to the documentation

Hi,

We are an academic team of software engineering researchers from a university working on automated program analysis techniques to improve API documentation quality, ultimately contributing to improving data science software development practices. we would like to keep anonymity for the purpose of double-blind paper reviewing.

We discover an inconsistency issue between documentation and code in the class [`sklearn.neighbors.NeighborhoodComponentsAnalysis`](https://scikit-learn.org/dev/modules/generated/sklearn.neighbors.NeighborhoodComponentsAnalysis.html#neighborhoodcomponentsanalysis). As mentioned in the description of the value `auto` of parameter `init`:

> -  **'auto'**
Depending on `n_components`, the most reasonable initialization will be chosen. **If `n_components <= n_classes` we use `'lda'`**, as it uses labels information. If not, but `n_components < min(n_features, n_samples)`, we use `'pca'`, as it projects data in meaningful directions (those of higher variance). Otherwise, we just use `'identity'`.

However, the most relevant source code snippet looks like this:

```python
if init == "auto":
    n_classes = len(np.unique(y))
    if n_components <= min(n_features, n_classes - 1):
        init = "lda"
    elif n_components < min(n_features, n_samples):
        init = "pca"
    else:
        init = "identity"
```

Apparently, the condition (**_n_components <= n_classes_**) shown in the document are different from the condition (**_n_components <= min(n_features, n_classes - 1)_**) actually executed in the code.


### Suggest a potential alternative/fix

Maybe you can update the documentation to avoid unnecessary misunderstanding.

COMMENT:
@DocInspector 's bio on GH says:

> We prefer to remain anonymous for double-blind paper review.

I'd say we prefer to talk to humans and we don't appreciate robots and fully automated tools. We also don't appreciate being used as validation to tools being developed out there. We're not free data collection agents for your automated tools.

I'm going to close this issue and mark it as spam, and will block the user since it's clearly not a known human. Feel free to open a PR fixing issues if you find any, using a normal human backed account.

Labels: Documentation, spam