put in version_added label by reshamas · Pull Request #15973 · scikit-learn/scikit-learn

reshamas · 2019-12-26T13:41:20Z

Reference Issues/PRs

Fixes part of Issue #15426
Closes #15482

What does this implement/fix? Explain your changes.

Towards the upcoming release of v0.22, it would be useful if some contributors (or sprinters) scoured the change log and identified whether any added/changed parameters/classes needed a versionadded or versionchanged label, and to produce pull requests adding them when otherwise omitted.

Any other comments?

Continuing work begun by @alekslovesdata at SF scikit sprint (Nov 2019)

Tversions

This reverts commit ef7d51f.

This reverts commit 56226ec.

…it-learn into version_docstrings

…rsClassifier: Issue #15426

Tversions

Version docstrings

…roupOut, LeavePGroupsOut: Issue #15426

…into adding_version

reshamas · 2019-12-26T19:30:02Z

@rth @jnothman
This PR is ready for review.

I could not figure out why the _target.py file was throwing the linting error, so removed it. I can add it back in if you have any advice on the formatting to update.

@cmarmo this is the last PR from the SF sprint.

cmarmo · 2019-12-26T22:23:20Z

@reshamas , the double * is interpreted by sphinx as a bold formatting start without end-formatting.
To avoid the warning you could write ``**param``. This will be rendered as **param: I'm not sure this is standard for skl documentation, but the warning will be gone.

reshamas · 2019-12-26T23:43:26Z

thanks @cmarmo. that worked! 👏

cmarmo · 2019-12-29T12:55:54Z

@jnothman , @amueller , @TomDLT , you reviewed the original PR.
Could you please check this one, as we can close the SF sprint list? Thanks for your collaboration!

jnothman

Thanks for the work.

Several version labels are incorrect, so I cannot trust the method used to produce them.

jnothman · 2020-01-03T04:18:02Z

sklearn/compose/_target.py

        -------
        self : object
+
+        .. versionchanged:: 0.22


Numpydoc won't process this correctly, since it's in the Returns section

I'm not sure whether we need such notes, but they have to appear in a Notes section or in the top description section.

I don't think such a bug fix requires that note anyway.

sklearn/compose/_target.py

sklearn/dummy.py

sklearn/ensemble/_gb_losses.py

sklearn/feature_extraction/_dict_vectorizer.py

jnothman · 2020-01-03T04:21:04Z

sklearn/feature_extraction/text.py

        out of the raw, unprocessed input.

-        .. versionchanged:: 0.21
+        .. versionadded:: 0.21


analyzer was added long before version 0.21. It should not be marked as added in 0.21.

I'm not sure what this versionchanged intends to refer to, and unless its referent can be clarified, the versioning note should be removed.

Ah... I think the versionchanged is with reference to the following content, which should be indented.

This was marked as resolved but @jnothman's comment still holds: the analyzer param was not added in 0.21, it is much older. It was changed in 0.21 as explained in the next paragraph. The previous annotation was correct.

I marked things as resolved as I fixed them, but I coudn't push to the repo. Created a new PR: #16015

sklearn/feature_extraction/text.py

jnothman · 2020-01-03T04:22:00Z

sklearn/feature_extraction/text.py

        If a callable is passed it is used to extract the sequence of features
        out of the raw, unprocessed input.

-        .. versionchanged:: 0.21


the versionchanged is with reference to the following content, which should be indented.

jnothman · 2020-01-03T04:29:40Z

sklearn/feature_extraction/text.py

                   Introduction to Information Retrieval. Cambridge University
                   Press, pp. 118-120.
+
+    .. versionadded:: 0.19


Firstly, this should not be in the References section.

Secondly, TfidfTransformer was not added as late as version 0.19. It was added in version 0.5 as far as I can tell!

How did you decide it was added in 0.19, and is it possible you could check for similar errors.

sklearn/linear_model/_bayes.py

glemaitre · 2020-01-03T14:40:31Z

I'll make a second review once @jnothman are addressed.

reshamas · 2020-01-03T14:46:08Z

I propose closing this PR which I took over from SF sprint.

I think it is best practice to not include that many files in one PR
For the upcoming Berlin beginner sprint, suggestion for new contributors is:

include only 2 files in each PR
link to the change log in the comments section so it can be verified.

cc: @adrinjalali @cmarmo

adrinjalali · 2020-01-03T14:48:00Z

I agree.

glemaitre · 2020-01-03T14:54:49Z

I propose closing this PR

It is costly on the reviewing side since @jnothman already make an entire review.

I agree that the PR is not ideal but I would prefer to quickly fix the comments and merge.

For the next PR, we could ask to cut the PR into smaller PRs before starting the review process.

cmarmo · 2020-01-03T15:07:29Z

I propose closing this PR

It is costly on the reviewing side since @jnothman already make an entire review.

@glemaitre, you are right, but the problem here is that @reshamas is not the original author of the PR and some of the comments are related to choices made by the first author.
I think that @jnothman deserves our excuses, and we all have probably to learn how more efficiently manage PR from beginner sprints.
I will reorganize the corresponding issue #15426 for the next two sprints... and ensure that @jnothman recommendations will be taken into account. Is that solution satisfying?

adrinjalali · 2020-01-03T15:10:07Z

I'll take over and apply the requested changes for us to close this PR, that's probably easier.

cmarmo · 2020-01-03T15:16:28Z

I'll take over and apply the requested changes for us to close this PR, that's probably easier.

As you wish, but this is indeed a nice beginner's issue because it forces to look in the scikit-learn API.
Let's see if @jnothman will forgive us, first? :)

adrinjalali · 2020-01-03T15:23:24Z

hmm, seems like maintainers can't update this PR?

reshamas · 2020-01-03T15:26:10Z

This PR is marked with "allow edits from maintainers".
not sure about the previous PR.

glemaitre · 2020-01-09T14:31:17Z

Closing this PR. We put back the estimator in the pool of the main issue for the next sprint.

Circe McDonald and others added 30 commits November 2, 2019 11:37

TfidfTransformer versionadded and versionchange annotated

fbbe27a

TheilSenRegressor versionadded

edd84ae

TimeSeriesSplit versionadded and formatting

9801353

TruncatedSVD versionadded

60b20be

put in version_added label: Issue #15426

79903f2

Merge pull request #1 from MZeej/tversions

b8d200d

Tversions

added versionchanged label to TfidfVectorizer

ef7d51f

Revert "added versionchanged label to TfidfVectorizer"

56226ec

This reverts commit ef7d51f.

Revert "Revert "added versionchanged label to TfidfVectorizer""

19deab2

This reverts commit 56226ec.

added versionchanged label to TfidifVectorizer

f19bb78

putting in adding_version label for AffinityPropogation: Issue #15426

74a473f

Merge branch 'version_docstrings' of https://github.com/EricaXia/scik…

6807064

…it-learn into version_docstrings

put in version_added label for RadiusNeighborsClassifier and KNeighbo…

1dd5bba

…rsClassifier: Issue #15426

added versionchanged label to TransformedTargetRegressor

355f83f

add versionchanged label to CheckingClassifier

3c90809

ComplementNB and MultinomialNB

7947a5f

add versionadded label to ClassificationLossFunction

78cf37f

added versionadded label to ClassifierChain

a4c1d59

put in version_added label for BernoulliRBM: Issue #15426

a556f41

put in version_added label for CCA: Issue #15426

f4f4ec8

put in version_added label for CalibratedClassifierCV: Issue #15426

1c7ed7c

CountVectorizer versionadded

63a9cc8

add versionchanged label to DictVectorizer

360052d

put in version_added label for CategoricalNB: Issue #15426

80d494d

DummyRegressor version added

8ae21cd

Minor correction

c3286d2

Merge pull request #2 from MZeej/tversions

e5044b9

Tversions

Merge pull request #3 from EricaXia/version_docstrings

4aaee26

Version docstrings

put in version_added label for GroupKFold, StratifiedKFold, LeaveOneG…

209b1b8

…roupOut, LeavePGroupsOut: Issue #15426

put in version_added label for VarianceThreshold: Issue #15426

05bde40

Reshama added 11 commits December 26, 2019 09:00

removing 'merging my work..' file

cbfadcd

fixed spacing errors

4395c7c

fixed linting error in _dict_vectorizer file

31877e8

fix linting error in _gb_losses.py

7fb96e6

removed whitespace in _truncated_svd.py

898997f

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

805cf6c

…into adding_version

fixed linting error in _target.py

d7ef028

try fixing error by adding blank line

7deded7

remove whitespace in _target.py

cf1589d

added period to end of sentence in versionadded text

f9e568e

reverting _target.py file for now

d22f661

adding _target.py file back; updating formatting of parameter

d9c2e35

jnothman requested changes Jan 3, 2020

View reviewed changes

adrinjalali mentioned this pull request Jan 3, 2020

adding versionadded to a few modules #16015

Closed

jnothman mentioned this pull request Jan 9, 2020

Label docstrings with versionadded / versionchanged #15426

Closed

glemaitre closed this Jan 9, 2020

Uh oh!

Conversation

reshamas commented Dec 26, 2019

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

reshamas commented Dec 26, 2019

Uh oh!

cmarmo commented Dec 26, 2019

Uh oh!

reshamas commented Dec 26, 2019

Uh oh!

cmarmo commented Dec 29, 2019

Uh oh!

jnothman 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glemaitre commented Jan 3, 2020

Uh oh!

reshamas commented Jan 3, 2020

Uh oh!

adrinjalali commented Jan 3, 2020

Uh oh!

glemaitre commented Jan 3, 2020

Uh oh!

cmarmo commented Jan 3, 2020

Uh oh!

adrinjalali commented Jan 3, 2020

Uh oh!

cmarmo commented Jan 3, 2020

Uh oh!

adrinjalali commented Jan 3, 2020

Uh oh!

reshamas commented Jan 3, 2020

Uh oh!

glemaitre commented Jan 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants