Skip to content

[MRG] DOC update UG and docstrings for isotonic regression#16234

Merged
agramfort merged 10 commits intoscikit-learn:masterfrom
NicolasHug:isotonic_docs
Feb 1, 2020
Merged

[MRG] DOC update UG and docstrings for isotonic regression#16234
agramfort merged 10 commits intoscikit-learn:masterfrom
NicolasHug:isotonic_docs

Conversation

@NicolasHug
Copy link
Copy Markdown
Member

also removed math equations from docstrings (some are outdated, some are wrong)

Copy link
Copy Markdown
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.

Haven't checked the math, otherwise there are some still left in the formattings which need to be fixed, the T_ on the transform method for instance. Sorry I meant to get to this, you got there faster.

The string value "auto" determines whether y should
increase or decrease based on the Spearman correlation estimate's
sign.
increasing : bool or 'auto', default= True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
increasing : bool or 'auto', default= True
increasing : bool or 'auto', default=True

@NicolasHug
Copy link
Copy Markdown
Member Author

the T_ on the transform method for instance

We cannot change that though, can we? The current signature is transform(self, T) so we'd be breaking backward compat if people use named parameters

Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Do we talk about increasing='auto' in the user guide?

@NicolasHug
Copy link
Copy Markdown
Member Author

Closes #16329

Comments were addressed, does this have your +1 @adrinjalali @thomasjpfan ?

@adrinjalali
Copy link
Copy Markdown
Member

There are still some left in the other places in the file, for instance:

        T_ : array, shape=(n_samples,)

I think last time I checked there were a few ones which needed to be fixed

Copy link
Copy Markdown
Contributor

@allefeld allefeld left a comment

Choose a reason for hiding this comment

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

Looks good generally, minor comments.

y_min : float, default=None
If not None, set the lowest value of the fit to y_min.
Lower bound on the lowest predicted value (the minimum value may
still be higher). Defaults to -inf.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it might be confusing that it first says default=None, but then Defaults to -inf. How about "If not None, lower bound ..." or "If specified, lower bound ..."

Returns
-------
T_ : array, shape=(n_samples,)
y_pred : ndarry of shape (n_samples,)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo ndarry

@allefeld
Copy link
Copy Markdown
Contributor

First time I did a review on GH – I chose "approve" though I had comments, not sure whether that's ok?

@NicolasHug
Copy link
Copy Markdown
Member Author

Thanks @allefeld !

I chose "approve" though I had comments, not sure whether that's ok?

Sure. I personally press approve when comments are minor, or when I know they'll be easily addressed so I don't need to check back

@agramfort agramfort merged commit 406184e into scikit-learn:master Feb 1, 2020
thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
…arn#16234)

* Isotonic regression docs

* remove space

* Apply suggestions from code review

Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>

* Documented auto option in UG

* fixed more docstrings

* Addressed comments

Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
…arn#16234)

* Isotonic regression docs

* remove space

* Apply suggestions from code review

Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>

* Documented auto option in UG

* fixed more docstrings

* Addressed comments

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants