-
-
Notifications
You must be signed in to change notification settings - Fork 25k
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.
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
DOC use the arxiv directive in the docstrings #21418
Conversation
- removed comment since it overlaps with docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you address all the remaining arxiv links in one single PR?
If not please put the names of the code / doc modules in the title of the PR so that others can see more specifically what this PR is about.
Thanks @SultanOrazbayev the lint check is failing with the following error
Do you mind fixing the check? Thanks! |
blank line contains whitespace
Sorry about this, I was using the web editor, so this slipped through. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aufarkari , thanks for your follow-up. You need to fix conflicts before adding new modifications. You might want to have a look to the github documentation for this.
Noted. I will edit accordingly.
…On Sun, Dec 19, 2021 at 9:37 PM Chiara Marmo ***@***.***> wrote:
***@***.**** commented on this pull request.
Hi @aufarkari <https://github.com/aufarkari> , thanks for your follow-up.
You need to fix conflicts before adding new modifications. You might want
to have a look to the github documentation
<https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line>
for this.
------------------------------
In sklearn/cluster/_hierarchical_fast.pyx
<#21418 (comment)>
:
> @@ -451,7 +451,7 @@ def single_linkage_label(L):
return _single_linkage_label(L)
-# Implements MST-LINKAGE-CORE from https://arxiv.org/abs/1109.2378
+# Implements MST-LINKAGE-CORE from :arxiv:`<1109.2378>`
This link is commented out, then not rendered in the documentation (see my previous
comment
<#21418 (comment)>
).
I believe it is better to revert this change, otherwise the link is not
copy-pastable.
------------------------------
In sklearn/ensemble/_hist_gradient_boosting/splitting.pyx
<#21418 (comment)>
:
> @@ -500,7 +500,7 @@ cdef class Splitter:
# case yields the best gain: either missing values go to
# the right (left to right scan) or to the left (right to
# left case). See algo 3 from the XGBoost paper
- # https://arxiv.org/abs/1603.02754
+ # :arxiv:`<1603.02754>`
Same comment as before.
—
Reply to this email directly, view it on GitHub
<#21418 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF5XLH2MX7S3TYX45DNQ3ZTUR2JFZANCNFSM5GSMA3GQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still have some lint errors:
sklearn/neural_network/_multilayer_perceptron.py:1024:70: W291 trailing whitespace
sklearn/neural_network/_multilayer_perceptron.py:1492:89: E501 line too long (92 > 88 characters)
sklearn/neural_network/_multilayer_perceptron.py:1495:70: W291 trailing whitespace
sklearn/neural_network/_stochastic_optimizers.py:240:89: E501 line too long (108 > 88 characters)
They prevent the build of the documentation making more difficult the review.
Hi @aufarkari , thanks for following up. Some conflicts arose, a synchronization with upstream main is needed to allow all the checks to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM given that the CI passes.
Thanks @SultanOrazbayev All is good with the CIs. Let's merge. |
Thank you very much! This was a joint effort with @aufarkari. Great to make a small contribution to scikit-learn! |
Nice work to both you then :) |
Co-authored-by: aufarkari <aufar.di.sini@gmail.com> Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
References #21088
:arxiv:
What does this implement/fix? Explain your changes.
Update docstrings to use consistent naming of
:arxiv:
links.Any other comments?
Together with @aufarkari we updated the
:arxiv:
links only (so:doi:
still need to be updated in a separate PR).