[MRG] DOC add versionadded versionchanged for 0.21 by gorski-m · Pull Request #16737 · scikit-learn/scikit-learn

gorski-m · 2020-03-21T10:39:45Z

Part of work in #15426.

I think there shouldn't have been anything added in files:
sklearn/compose/_column_transformer.py
sklearn/ensemble/_voting.py
but I decided to include these changes too. I will just remove them if necessary.

gorski-m · 2020-03-28T09:58:41Z

@jnothman @adrinjalali @cmarmo could you have a look?

adrinjalali

I didn't check for completeness, but these changes look good to me, except the two notes I'm not sure about. Thanks @Reksbril

I'll wait for another review before changing what you have.

adrinjalali · 2020-03-29T10:07:23Z

sklearn/cluster/_agglomerative.py

+        .. versionchanged:: 0.21
+            ``n_components_`` was renamed to ``n_connected_components_``.
+


I'm not sure if we need these. Its true that the attribute was renamed, but from the perspective of n_connected_components_, nothing has changed since its introduction. Not sure if we should do a versionadded instead or not. WDYT @jnothman ?

I feel like these might be useful, e.g. for a user to understand why and when their code started to break. No strong opinion though

Maybe versionadded and:

n_connected_components_ was added to replace n_components_

+1 for the @thomasjpfan versionadded formulation.

sklearn/compose/_column_transformer.py

gorski-m · 2020-04-16T14:50:36Z

@jnothman @cmarmo could you have a look?

gorski-m · 2020-04-24T07:12:16Z

@jnothman @cmarmo ping

NicolasHug

Thanks for the PR and for your patience @Reksbril

Made a few comments but mostly looks good. Just not sure about the usefulness of some of these

NicolasHug · 2020-04-24T21:17:48Z

sklearn/cluster/_agglomerative.py

+        .. versionchanged:: 0.21
+            ``n_components_`` was renamed to ``n_connected_components_``.
+


I feel like these might be useful, e.g. for a user to understand why and when their code started to break. No strong opinion though

sklearn/compose/_column_transformer.py

sklearn/ensemble/_voting.py

NicolasHug · 2020-04-24T21:21:56Z

sklearn/neighbors/_base.py

        """Finds the K-neighbors of a point.
        Returns indices of and distances to the neighbors of each point.

+        .. versionchanged:: 0.21


I feel like we wouldn't need these. Thoughts @adrinjalali ?

I agree we do not need the versionchanged for NotFittedError.

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

NicolasHug

Thanks @Reksbril

LGTM, though I think we don't need the NotFittedError entries. Let's see what others think

gorski-m · 2020-05-10T11:58:14Z

@adrinjalali @ThomasDelteil could you check?

cmarmo · 2020-05-10T12:14:51Z

Hi @Reksbril could you please modify the n_connected_components_ directive as proposed in @thomasjpfan comment? Then LGTM for me. Thanks!

gorski-m · 2020-05-10T12:20:06Z

@cmarmo oh, thanks, I've missed this one

cmarmo · 2020-05-10T12:23:46Z

sklearn/cluster/_agglomerative.py

        The estimated number of connected components in the graph.

+        .. versionchanged:: 0.21
+            ``n_components_`` was renamed to ``n_connected_components_``


Should be modified as above (versionchanged -> versionadded)

gorski-m · 2020-05-10T14:53:13Z

@cmarmo I've made the requested change

cmarmo · 2020-05-10T16:17:56Z

@thomasjpfan this is the last PR to close #15426. I suppose failing checks are related to the current work on check_estimator ... Shall we merge this one? Thanks!

thomasjpfan · 2020-05-10T19:49:09Z

Thank you for the ping @cmarmo and thank you for the PR @Reksbril !

Co-authored-by: Mateusz Górski <mateuszg122@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>

DOC add versionadded versionchanged for 0.21

f6d7fc7

gorski-m changed the title ~~DOC add versionadded versionchanged for 0.21~~ [WIP] DOC add versionadded versionchanged for 0.21 Mar 21, 2020

adrinjalali reviewed Mar 29, 2020

View reviewed changes

cmarmo added Needs work Needs Decision Requires decision and removed Needs work labels Apr 14, 2020

NicolasHug reviewed Apr 24, 2020

View reviewed changes

gorski-m and others added 2 commits April 27, 2020 08:54

Remove versionchanged

e445a2b

Apply suggestions from code review

5644027

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

github-actions bot added module:cluster module:ensemble module:neighbors labels Apr 27, 2020

gorski-m requested review from NicolasHug and adrinjalali April 28, 2020 09:23

NicolasHug approved these changes Apr 28, 2020

View reviewed changes

NicolasHug changed the title ~~[WIP] DOC add versionadded versionchanged for 0.21~~ [MRG] DOC add versionadded versionchanged for 0.21 Apr 28, 2020

cmarmo removed the Needs Decision Requires decision label Apr 28, 2020

gorski-m added 2 commits April 29, 2020 11:51

Update _base.py

62afe27

Update _base.py

7ee5cfa

gorski-m requested a review from thomasjpfan April 29, 2020 09:54

cmarmo added the Waiting for Reviewer label May 4, 2020

gorski-m requested a review from cmarmo May 10, 2020 11:57

cmarmo approved these changes May 10, 2020

View reviewed changes

Update _agglomerative.py

bd80067

cmarmo reviewed May 10, 2020

View reviewed changes

cmarmo removed the Waiting for Reviewer label May 10, 2020

Update _agglomerative.py

c3b60b1

thomasjpfan added 2 commits May 10, 2020 12:23

Merge remote-tracking branch 'upstream/master' into pr/16737

57ceea8

Remove reference to None

b6797ab

thomasjpfan merged commit 534f8ae into scikit-learn:master May 10, 2020

cmarmo mentioned this pull request May 11, 2020

added versionadded 0.21 and versionchanged 0.21 where applicable, aft… #16214

Closed

		.. versionchanged:: 0.21
		``n_components_`` was renamed to ``n_connected_components_``.

Uh oh!

Conversation

gorski-m commented Mar 21, 2020

Uh oh!

gorski-m commented Mar 28, 2020

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gorski-m commented Apr 16, 2020

Uh oh!

gorski-m commented Apr 24, 2020

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

gorski-m commented May 10, 2020

Uh oh!

cmarmo commented May 10, 2020

Uh oh!

gorski-m commented May 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gorski-m commented May 10, 2020

Uh oh!

cmarmo commented May 10, 2020

Uh oh!

thomasjpfan commented May 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants