8000 [MRG] DOC add versionadded versionchanged for 0.21 by Reksbril · Pull Request #16737 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] DOC add versionadded versionchanged for 0.21 #16737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
May 10, 2020
Merged

[MRG] DOC add versionadded versionchanged for 0.21 #16737

merged 9 commits into from
May 10, 2020

Conversation

Reksbril
Copy link
Contributor

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.

@Reksbril Reksbril changed the title DOC add versionadded versionchanged for 0.21 [WIP] DOC add versionadded versionchanged for 0.21 Mar 21, 2020
@Reksbril
Copy link
Contributor Author

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

adrinjalali
adrinjalali reviewed Mar 29, 2020
View reviewed changes
Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sklearn/cluster/_agglomerative.py Outdated
Comment on lines 762 to 764
.. versionchanged:: 0.21
8000 ``n_components_`` was renamed to ``n_connected_components_``.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe versionadded and:

n_connected_components_ was added to replace n_components_

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for the @thomasjpfan versionadded formulation.

@cmarmo cmarmo added Needs work Needs Decision Requires decision and removed Needs work labels Apr 14, 2020
@Reksbril
Copy link
Contributor Author

@jnothman @cmarmo could you have a look?

@Reksbril
Copy link
Contributor Author

@jnothman @cmarmo ping

NicolasHug
NicolasHug reviewed Apr 24, 2020
View reviewed changes
Copy link
Member
@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sklearn/cluster/_agglomerative.py Outdated
Comment on lines 762 to 764
.. versionchanged:: 0.21
``n_components_`` was renamed to ``n_connected_components_``.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/neighbors/_base.py Outdated
@@ -534,6 +534,10 @@ def kneighbors(self, X=None, n_neighbors=None, return_distance=True):
"""Finds the K-neighbors of a point.
Returns indices of and distances to the neighbors of each point.

.. versionchanged:: 0.21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we do not need the versionchanged for NotFittedError.

Reksbril and others added 2 commits April 27, 2020 08:54
@Reksbril
Remove versionchanged
e445a2b
@Reksbril @NicolasHug
Apply suggestions from code review
5644027 
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
NicolasHug
NicolasHug approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member
@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Reksbril

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

@NicolasHug NicolasHug changed the title [WIP] DOC add versionadded versionchanged for 0.21 [MRG] DOC add versionadded versionchanged for 0.21 Apr 28, 2020
@cmarmo cmarmo removed the Needs Decision Requires decision label Apr 28, 2020
@Reksbril Reksbril requested a review from thomasjpfan April 29, 2020 09:54
@Reksbril Reksbril requested a review from cmarmo May 10, 2020 11:57
@Reksbril
Copy link
Contributor Author

@adrinjalali @ThomasDelteil could you check?

@cmarmo
Copy link
Contributor
cmarmo commented May 10, 2020

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

@Reksbril
Copy link
Contributor Author

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

cmarmo
cmarmo reviewed May 10, 2020
View reviewed changes
sklearn/cluster/_agglomerative.py Outdated
@@ -1003,6 +1006,9 @@ class FeatureAgglomeration(AgglomerativeClustering, AgglomerationTransform):
n_connected_components_ : int
The estimated number of connected components in the graph.

.. versionchanged:: 0.21
``n_components_`` was renamed to ``n_connected_components_``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be modified as above (versionchanged -> versionadded)

@Reksbril
Copy link
Contributor Author

@cmarmo I've made the requested change

@cmarmo
Copy link
Contributor
cmarmo commented May 10, 2020

@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 thomasjpfan merged commit 534f8ae into scikit-learn:master May 10, 2020
@thomasjpfan
Copy link
Member

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

@cmarmo cmarmo mentioned this pull request May 11, 2020
Closed
adrinjalali pushed a commit to adrinjalali/scikit-learn that referenced this pull request May 11, 2020
@Reksbril @NicolasHug
@thomasjpfan @adrinjalali
DOC add versionadded versionchanged for 0.21 (scikit-learn#16737)
538bebd 
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>
adrinjalali pushed a commit that referenced this pull request May 12, 2020
@Reksbril @NicolasHug
@thomasjpfan @adrinjalali
DOC add versionadded versionchanged for 0.21 (#16737)
1d48e52 
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>
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@Reksbril @NicolasHug
@thomasjpfan @gio8tisu
DOC add versionadded versionchanged for 0.21 (scikit-learn#16737)
c2b3e12 
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>
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@Reksbril @NicolasHug
@thomasjpfan @viclafargue
DOC add versionadded versionchanged for 0.21 (scikit-learn#16737)
a2cb014 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmarmo cmarmo cmarmo approved these changes

@NicolasHug NicolasHug NicolasHug approved these changes

@adrinjalali adrinjalali Awaiting requested review from adrinjalali

@thomasjpfan thomasjpfan Awaiting requested review from thomasjpfan

Assignees
No one assigned
Labels
module:cluster module:ensemble module:neighbors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Reksbril @cmarmo @thomasjpfan @NicolasHug @adrinjalali
0