Talip7/scikit-learn-issues-multilabel huggingface dataset:
RFC Enabling auto-merge builtin Github functionality

The main advantage would be to be able to say on a per-PR basis "this PR can be merged when CI is green". See the [doc](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request) for more details. cc @scikit-learn/core-devs @scikit-learn/contributor-experience-team for visibility.

Currently we need to remember to come back in one hour and we sometimes forget. As a maintainer with many different things to track, it would be very nice to have this sense of quicker completion and the peace of mind of not having to remember to come back to merge the PR.

Here is a screenshot from sphinx-gallery which has set this up:
![image](https://user-images.githubusercontent.com/1680079/204514285-836bca8f-163e-4f07-9a84-b13441b8ee6d.png)

You can see this is a per-PR thing ("Enable automerge" button) and there is even a way to bypass branch protection, which can be nice in some rare cases (for example we know for a fact that the CI error is not related to the PR).

Things we would need to do:
- [ ] set-up branch [branch protection rules](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule)
  + [ ] define required checks: probably azure + github doc build, maybe others. We probably want to leave aside other more noisy checks like codecov?
  + [ ] require 1 approval rather than 2 (to allow DOC PRs to be merged with only one approval)
- [ ] set-up [auto-merge](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-auto-merge-for-pull-requests-in-your-repository)
  + [ ] make sure we check the require required checks (Thomas' comment at the meeting)
  + [ ] reset auto-merge when a commit is pushed after auto-merge is set, just to be on the safe side. This [link](https://github.blog/changelog/2021-03-22-updated-pull-request-auto-merge-behavior-for-users-without-write-access/) says that it is done automatically, unclear whether this is the same thing as "Dismiss stale pull request approvals when new commits are pushed" setting in branch protection rules.

Other items from discussions at the meeting:
- Q: does a human have to click "auto merge" on a PR for it to happen, or does it automatically happen for all PRs that match the approval requirements?
- A: a per PR manual action is required to allow automerge, see screenshot above
- Q: is "merge when CI is green" functionality of "[Refined GitHub](https://github.com/refined-github/refined-github)" a solution?
- A: you need to keep the tab open for it to work, and in my limited experience it does not work reliably. Also it is not public information that the PR will be merged when CI is green.

COMMENT:
I don't think codecov is too noisy. I think we can deal with the few PRs where codecov blocks and manually merge those PRs once the rest of the CI is green.

Labels: RFC
RFC Enabling auto-merge builtin Github functionality

The main advantage would be to be able to say on a per-PR basis "this PR can be merged when CI is green". See the [doc](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request) for more details. cc @scikit-learn/core-devs @scikit-learn/contributor-experience-team for visibility.

Currently we need to remember to come back in one hour and we sometimes forget. As a maintainer with many different things to track, it would be very nice to have this sense of quicker completion and the peace of mind of not having to remember to come back to merge the PR.

Here is a screenshot from sphinx-gallery which has set this up:
![image](https://user-images.githubusercontent.com/1680079/204514285-836bca8f-163e-4f07-9a84-b13441b8ee6d.png)

You can see this is a per-PR thing ("Enable automerge" button) and there is even a way to bypass branch protection, which can be nice in some rare cases (for example we know for a fact that the CI error is not related to the PR).

Things we would need to do:
- [ ] set-up branch [branch protection rules](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule)
  + [ ] define required checks: probably azure + github doc build, maybe others. We probably want to leave aside other more noisy checks like codecov?
  + [ ] require 1 approval rather than 2 (to allow DOC PRs to be merged with only one approval)
- [ ] set-up [auto-merge](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-auto-merge-for-pull-requests-in-your-repository)
  + [ ] make sure we check the require required checks (Thomas' comment at the meeting)
  + [ ] reset auto-merge when a commit is pushed after auto-merge is set, just to be on the safe side. This [link](https://github.blog/changelog/2021-03-22-updated-pull-request-auto-merge-behavior-for-users-without-write-access/) says that it is done automatically, unclear whether this is the same thing as "Dismiss stale pull request approvals when new commits are pushed" setting in branch protection rules.

Other items from discussions at the meeting:
- Q: does a human have to click "auto merge" on a PR for it to happen, or does it automatically happen for all PRs that match the approval requirements?
- A: a per PR manual action is required to allow automerge, see screenshot above
- Q: is "merge when CI is green" functionality of "[Refined GitHub](https://github.com/refined-github/refined-github)" a solution?
- A: you need to keep the tab open for it to work, and in my limited experience it does not work reliably. Also it is not public information that the PR will be merged when CI is green.

COMMENT:
@lesteve: so far I am really satisfied with the auto-merge builtin GitHub functionality being enabled. Thank you for having proposed and activated it. :+1:

Labels: RFC
RFC Enabling auto-merge builtin Github functionality

The main advantage would be to be able to say on a per-PR basis "this PR can be merged when CI is green". See the [doc](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request) for more details. cc @scikit-learn/core-devs @scikit-learn/contributor-experience-team for visibility.

Currently we need to remember to come back in one hour and we sometimes forget. As a maintainer with many different things to track, it would be very nice to have this sense of quicker completion and the peace of mind of not having to remember to come back to merge the PR.

Here is a screenshot from sphinx-gallery which has set this up:
![image](https://user-images.githubusercontent.com/1680079/204514285-836bca8f-163e-4f07-9a84-b13441b8ee6d.png)

You can see this is a per-PR thing ("Enable automerge" button) and there is even a way to bypass branch protection, which can be nice in some rare cases (for example we know for a fact that the CI error is not related to the PR).

Things we would need to do:
- [ ] set-up branch [branch protection rules](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule)
  + [ ] define required checks: probably azure + github doc build, maybe others. We probably want to leave aside other more noisy checks like codecov?
  + [ ] require 1 approval rather than 2 (to allow DOC PRs to be merged with only one approval)
- [ ] set-up [auto-merge](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-auto-merge-for-pull-requests-in-your-repository)
  + [ ] make sure we check the require required checks (Thomas' comment at the meeting)
  + [ ] reset auto-merge when a commit is pushed after auto-merge is set, just to be on the safe side. This [link](https://github.blog/changelog/2021-03-22-updated-pull-request-auto-merge-behavior-for-users-without-write-access/) says that it is done automatically, unclear whether this is the same thing as "Dismiss stale pull request approvals when new commits are pushed" setting in branch protection rules.

Other items from discussions at the meeting:
- Q: does a human have to click "auto merge" on a PR for it to happen, or does it automatically happen for all PRs that match the approval requirements?
- A: a per PR manual action is required to allow automerge, see screenshot above
- Q: is "merge when CI is green" functionality of "[Refined GitHub](https://github.com/refined-github/refined-github)" a solution?
- A: you need to keep the tab open for it to work, and in my limited experience it does not work reliably. Also it is not public information that the PR will be merged when CI is green.

COMMENT:
Great, thanks for letting me know!

I am considering waiting a few more days, in case there are further feed-back, and then close this issue.

Labels: RFC
RFC Enabling auto-merge builtin Github functionality

The main advantage would be to be able to say on a per-PR basis "this PR can be merged when CI is green". See the [doc](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request) for more details. cc @scikit-learn/core-devs @scikit-learn/contributor-experience-team for visibility.

Currently we need to remember to come back in one hour and we sometimes forget. As a maintainer with many different things to track, it would be very nice to have this sense of quicker completion and the peace of mind of not having to remember to come back to merge the PR.

Here is a screenshot from sphinx-gallery which has set this up:
![image](https://user-images.githubusercontent.com/1680079/204514285-836bca8f-163e-4f07-9a84-b13441b8ee6d.png)

You can see this is a per-PR thing ("Enable automerge" button) and there is even a way to bypass branch protection, which can be nice in some rare cases (for example we know for a fact that the CI error is not related to the PR).

Things we would need to do:
- [ ] set-up branch [branch protection rules](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule)
  + [ ] define required checks: probably azure + github doc build, maybe others. We probably want to leave aside other more noisy checks like codecov?
  + [ ] require 1 approval rather than 2 (to allow DOC PRs to be merged with only one approval)
- [ ] set-up [auto-merge](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-auto-merge-for-pull-requests-in-your-repository)
  + [ ] make sure we check the require required checks (Thomas' comment at the meeting)
  + [ ] reset auto-merge when a commit is pushed after auto-merge is set, just to be on the safe side. This [link](https://github.blog/changelog/2021-03-22-updated-pull-request-auto-merge-behavior-for-users-without-write-access/) says that it is done automatically, unclear whether this is the same thing as "Dismiss stale pull request approvals when new commits are pushed" setting in branch protection rules.

Other items from discussions at the meeting:
- Q: does a human have to click "auto merge" on a PR for it to happen, or does it automatically happen for all PRs that match the approval requirements?
- A: a per PR manual action is required to allow automerge, see screenshot above
- Q: is "merge when CI is green" functionality of "[Refined GitHub](https://github.com/refined-github/refined-github)" a solution?
- A: you need to keep the tab open for it to work, and in my limited experience it does not work reliably. Also it is not public information that the PR will be merged when CI is green.

COMMENT:
It seems like auto-merge is working as expected, closing this one.

Labels: RFC
`KMeans.predict` wrongly exposes a `sample_weight` argument

### Describe the issue linked to the documentation

It's more an UX issue than a documentation issue maybe ?

`sample_weight` should not be passed to `predict`, weights of sample are not related to how far samples are from centers, which is what `predict` computes to find labels. `predict` output does not need `sample_weight` nor depends on what `sample_weight` is passed anyway.  

(another way to see it is that `sample_weight` is only used if `update_centers` is `True` in the [private function](https://github.com/scikit-learn/scikit-learn/blob/17df37aee774720212c27dbc34e6f1feef0e2482/sklearn/cluster/_k_means_lloyd.pyx#L223), and `update_centers` [is always `False`](https://github.com/scikit-learn/scikit-learn/blob/17df37aee/sklearn/cluster/_kmeans.py#L770) for prediction methods)

(Note that `KMeans.score` *does* require `sample_weight`)

### Suggest a potential alternative/fix

`sample_weight` should be removed from `predict` signature. Since it breaks UX, it requires a deprecation cycle.

COMMENT:
I agree, we should deprecate the ``sample_weights`` param from ``predict``

Labels: Bug
`KMeans.predict` wrongly exposes a `sample_weight` argument

### Describe the issue linked to the documentation

It's more an UX issue than a documentation issue maybe ?

`sample_weight` should not be passed to `predict`, weights of sample are not related to how far samples are from centers, which is what `predict` computes to find labels. `predict` output does not need `sample_weight` nor depends on what `sample_weight` is passed anyway.  

(another way to see it is that `sample_weight` is only used if `update_centers` is `True` in the [private function](https://github.com/scikit-learn/scikit-learn/blob/17df37aee774720212c27dbc34e6f1feef0e2482/sklearn/cluster/_k_means_lloyd.pyx#L223), and `update_centers` [is always `False`](https://github.com/scikit-learn/scikit-learn/blob/17df37aee/sklearn/cluster/_kmeans.py#L770) for prediction methods)

(Note that `KMeans.score` *does* require `sample_weight`)

### Suggest a potential alternative/fix

`sample_weight` should be removed from `predict` signature. Since it breaks UX, it requires a deprecation cycle.

COMMENT:
No PR yet?

Labels: Bug
`KMeans.predict` wrongly exposes a `sample_weight` argument

### Describe the issue linked to the documentation

It's more an UX issue than a documentation issue maybe ?

`sample_weight` should not be passed to `predict`, weights of sample are not related to how far samples are from centers, which is what `predict` computes to find labels. `predict` output does not need `sample_weight` nor depends on what `sample_weight` is passed anyway.  

(another way to see it is that `sample_weight` is only used if `update_centers` is `True` in the [private function](https://github.com/scikit-learn/scikit-learn/blob/17df37aee774720212c27dbc34e6f1feef0e2482/sklearn/cluster/_k_means_lloyd.pyx#L223), and `update_centers` [is always `False`](https://github.com/scikit-learn/scikit-learn/blob/17df37aee/sklearn/cluster/_kmeans.py#L770) for prediction methods)

(Note that `KMeans.score` *does* require `sample_weight`)

### Suggest a potential alternative/fix

`sample_weight` should be removed from `predict` signature. Since it breaks UX, it requires a deprecation cycle.

COMMENT:
I don't think there is. Contribution welcome if you want to give it a try !

Labels: Bug
`parameters_to_validate` to allow for partial validation in validation framework

In some PRs for adding validation to functions: https://github.com/scikit-learn/scikit-learn/pull/24924 or https://github.com/scikit-learn/scikit-learn/pull/25026, there will be double validation, by the function and another by the estimator. @adrinjalali and I share the same concern about keeping constraints in sync between the function and the estimator: https://github.com/scikit-learn/scikit-learn/pull/24924#discussion_r1029871876, https://github.com/scikit-learn/scikit-learn/pull/25026#issuecomment-1328780076.

The test for validation is very strict about including a constraint for every parameter. I see the purpose of having this strictness is such that when a new parameter is added, then a constraint is also included.

I propose adding a `parameters_to_validate` to `validate_params` which is a collection of strings that states which parameters should be validated. This way when a new parameter is added, the default is to add a constraint for the new parameter, but it can be ignored by adding it to the `parameters_to_validate` list.

CC @jeremiedbb @glemaitre @adrinjalali

COMMENT:
instead of `parameters_to_validate`, we can also add something like `param: "ignore"`, and have that meaning "this method doesn't validate this parameter, but something it calls does"

Labels: New Feature, Needs Decision - Include Feature
`parameters_to_validate` to allow for partial validation in validation framework

In some PRs for adding validation to functions: https://github.com/scikit-learn/scikit-learn/pull/24924 or https://github.com/scikit-learn/scikit-learn/pull/25026, there will be double validation, by the function and another by the estimator. @adrinjalali and I share the same concern about keeping constraints in sync between the function and the estimator: https://github.com/scikit-learn/scikit-learn/pull/24924#discussion_r1029871876, https://github.com/scikit-learn/scikit-learn/pull/25026#issuecomment-1328780076.

The test for validation is very strict about including a constraint for every parameter. I see the purpose of having this strictness is such that when a new parameter is added, then a constraint is also included.

I propose adding a `parameters_to_validate` to `validate_params` which is a collection of strings that states which parameters should be validated. This way when a new parameter is added, the default is to add a constraint for the new parameter, but it can be ignored by adding it to the `parameters_to_validate` list.

CC @jeremiedbb @glemaitre @adrinjalali

COMMENT:
>  we can also add something like param: "ignore"

We already have that :)
``"param": "no_validation"``

I think it would be the most appropriate because it really means delegate validation to whatever uses this param. For instance, we use it for the ``dtype`` param and delegate validation to numpy. Here it would mean delegate validation to the class.

Labels: New Feature, Needs Decision - Include Feature
`parameters_to_validate` to allow for partial validation in validation framework

In some PRs for adding validation to functions: https://github.com/scikit-learn/scikit-learn/pull/24924 or https://github.com/scikit-learn/scikit-learn/pull/25026, there will be double validation, by the function and another by the estimator. @adrinjalali and I share the same concern about keeping constraints in sync between the function and the estimator: https://github.com/scikit-learn/scikit-learn/pull/24924#discussion_r1029871876, https://github.com/scikit-learn/scikit-learn/pull/25026#issuecomment-1328780076.

The test for validation is very strict about including a constraint for every parameter. I see the purpose of having this strictness is such that when a new parameter is added, then a constraint is also included.

I propose adding a `parameters_to_validate` to `validate_params` which is a collection of strings that states which parameters should be validated. This way when a new parameter is added, the default is to add a constraint for the new parameter, but it can be ignored by adding it to the `parameters_to_validate` list.

CC @jeremiedbb @glemaitre @adrinjalali

COMMENT:
> We already have that :) "param": "no_validation"

Ah yes. Moving forward, I will start recommending `"no_validation"` in cases where there is double validation (with a inline comment that states that the validation is delegated).

Labels: New Feature, Needs Decision - Include Feature
`parameters_to_validate` to allow for partial validation in validation framework

In some PRs for adding validation to functions: https://github.com/scikit-learn/scikit-learn/pull/24924 or https://github.com/scikit-learn/scikit-learn/pull/25026, there will be double validation, by the function and another by the estimator. @adrinjalali and I share the same concern about keeping constraints in sync between the function and the estimator: https://github.com/scikit-learn/scikit-learn/pull/24924#discussion_r1029871876, https://github.com/scikit-learn/scikit-learn/pull/25026#issuecomment-1328780076.

The test for validation is very strict about including a constraint for every parameter. I see the purpose of having this strictness is such that when a new parameter is added, then a constraint is also included.

I propose adding a `parameters_to_validate` to `validate_params` which is a collection of strings that states which parameters should be validated. This way when a new parameter is added, the default is to add a constraint for the new parameter, but it can be ignored by adding it to the `parameters_to_validate` list.

CC @jeremiedbb @glemaitre @adrinjalali

COMMENT:
I'm reopening because it's actually not fully satisfying.

If we put `"no_validation"`, when the user gives an invalid param, the error message will be `The <param_name> parameter of NMF should be` but the user is actually calling `non_negative_factorization`. I think it brings confusion. Wdyt ?

Labels: New Feature, Needs Decision - Include Feature
`parameters_to_validate` to allow for partial validation in validation framework

In some PRs for adding validation to functions: https://github.com/scikit-learn/scikit-learn/pull/24924 or https://github.com/scikit-learn/scikit-learn/pull/25026, there will be double validation, by the function and another by the estimator. @adrinjalali and I share the same concern about keeping constraints in sync between the function and the estimator: https://github.com/scikit-learn/scikit-learn/pull/24924#discussion_r1029871876, https://github.com/scikit-learn/scikit-learn/pull/25026#issuecomment-1328780076.

The test for validation is very strict about including a constraint for every parameter. I see the purpose of having this strictness is such that when a new parameter is added, then a constraint is also included.

I propose adding a `parameters_to_validate` to `validate_params` which is a collection of strings that states which parameters should be validated. This way when a new parameter is added, the default is to add a constraint for the new parameter, but it can be ignored by adding it to the `parameters_to_validate` list.

CC @jeremiedbb @glemaitre @adrinjalali

COMMENT:
@glemaitre also found that having long lists of "no_validation" was non very satisfying either and can be error prone because if a contributor put "no_validation" for a parameter that is actually not in the class, then it will never be validated and no test would catch that.

Thus we thought that we could allow to only pass a fraction of the parameters to the function constraint dict and the common test would take the constraint dict of the class and the function, merge them (giving priority to the function for duplicated entries), and then proceed with the checks.

(It does solve the issue I raised in the previous message though)

Labels: New Feature, Needs Decision - Include Feature