-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC describe scikit-learn-contrib in related projects and contributing docs #8440
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8440 +/- ##
==========================================
+ Coverage 95.47% 95.47% +<.01%
==========================================
Files 342 342
Lines 60902 60907 +5
==========================================
+ Hits 58149 58154 +5
Misses 2753 2753
Continue to review full report at Codecov.
|
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.
Small comments, otherwise LGTM
doc/developers/contributing.rst
Outdated
by deriving a class from ``BaseEstimator`` | ||
and optionally the mixin classes in ``sklearn.base``. | ||
E.g., below is a custom classifier. | ||
`scikit-learn-contrib <https://github.com/scikit-learn-contrib/project-template/blob/master/skltemplate/template.py>`_:: |
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 removed "For more information on this example see", was that intentional? Now scikit-learn-contrib stand on its own which is a bit weird.
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.
I meant to remove "for more information on this example" because I found it weird. I did not mean to leave the URL there. WDYT?
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.
Looks like the template has some other examples of estimators, maybe you can do:
E.g., below is a custom classifier
>>> import numpy as np
...
For more examples of estimators see the `scikit-learn-contrib template <url>`_
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.
Thanks
doc/developers/contributing.rst
Outdated
selection tools such as :class:`model_selection.GridSearchCV` and | ||
:class:`pipeline.Pipeline`. | ||
|
||
For this to work, you need to implement the interface described below. |
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.
Not sure what "described below" actually refers to, can we add a link? I know it was like this before but adding topics as you did makes below even belower if that makes sense ...
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.
How about: "Before detailing the required interface below, we describe two ways to achieve the correct interface more easily."
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.
sounds good
Changes made, thanks @lesteve |
LGTM, merging thanks a lot! |
No description provided.