Separate documentation for contributing and developing estimators in to different files #14582 by arpanchowdhry · Pull Request #14611 · scikit-learn/scikit-learn

arpanchowdhry · 2019-08-09T06:17:18Z

Addresses #14582

I used the following as a guideline to separate sections. Also this is just an initial version to get feedback. The new file with developing estimators is pretty bare bones and might need more verbiage.

Ways to contribute - Contributing
Submitting a bug report or a feature request - Contributing
How to make a good bug report
Contributing code - Contributing
How to contribute
Pull request checklist
Continuous Integration (CI)
Stalled pull requests
Issues for New Contributors
Documentation - Contributing
Building the documentation
Guidelines for writing documentation
Generated documentation on CircleCI
Testing and improving test coverage - Developing
Writing matplotlib related tests
Workflow to improve test coverage
Issue Tracker Tags - Contributing - I don't think this belongs here in any case
Coding guidelines - Contributing for top matter
Input validation - Developing?
Random Numbers - Developing
Deprecation - Contributing
Change the default value of a parameter - Contributing
Python versions supported - Contributing
Code Review Guidelines - Contributing
APIs of scikit-learn objects - Developing
Different objects
Estimators
Instantiation
Fitting
Estimated Attributes
Optional Arguments
Pairwise Attributes
Rolling your own estimator - Developing
get_params and set_params
Parameters and init
Cloning
Pipeline compatibility
Estimator types
Specific models
Estimator Tags
Reading the existing code base - Contributing

…n to different files

NicolasHug

Thanks for taking care of this @arpanchowdhry .

Just a few comments for now. You will also need to add the reference to doc/developers/index.rst for the new file to be rendered.

NicolasHug · 2019-08-09T12:33:37Z

doc/developers/developing_estimators.rst

+.. _api_overview:
+
+APIs of scikit-learn objects
+============================


Maybe this should come first?

NicolasHug · 2019-08-09T12:34:28Z

doc/developers/developing_estimators.rst

@@ -0,0 +1,761 @@
+.. _developing_estimators:


This section is intended to be about development in general, not just developing estimator. Maybe change the ref/file/section to e.g. "Developing scikit-learn" ? (not great either, others will have better ideas)

…inor changes

arpanchowdhry · 2019-08-10T18:22:38Z

@NicolasHug Hi, I made the required changes, but I am seeing all sorts for failures due the reference addition in index.rst. Any thoughts?

NicolasHug · 2019-08-12T12:31:13Z

I think the CI is going berserk lately, your docs build correctly on my machine.

jnothman

Thanks for this. It does need a little more work.

jnothman · 2019-08-13T03:04:43Z

doc/developers/developing_scikit.rst

@@ -0,0 +1,761 @@
+.. _developing_scikit:
+
+============


Please extend these lines to match the length of the text

jnothman · 2019-08-13T03:05:42Z

doc/developers/developing_scikit.rst

+.. _developing_scikit:
+
+============
+Developing scikit-learn


Suggested change

Developing scikit-learn

Developing scikit-learn estimators

doc/developers/developing_scikit.rst

jnothman · 2019-08-13T03:13:15Z

doc/developers/developing_scikit.rst

+3. Loop.
+
+
+Issue Tracker Tags


I'm pretty sure this belongs in the contributer guide, not here: it is about the specific development processes for the scikit-learn project

jnothman · 2019-08-13T03:19:55Z

doc/developers/developing_scikit.rst

+Coding guidelines
+=================
+
+The following are some guidelines on how new code should be written. Of


Thesl issue suggests that the top matter from this section goes into the contributing guide, not here. Or maybe we can just say that the following are some standards used within scikit-learn, which may be appropriate to adopt in external projects for the sake of consistency

@jnothman Thanks for the feedback. I have made all the requested changes. But I am still not sure about how to properly handle the 'Coding guidelines' section in the new file. I just changed the first line to reflect what you had commented above. But if further modifications are needed please let me know.

jnothman

A few more.

jnothman · 2019-08-13T08:33:53Z

doc/developers/developing_scikit.rst

+Coding guidelines
+=================
+
+The following  are some standards used within scikit-learn, which may


Despite being my words, I'm not happy with this result. Let's use the existing "The following are some guidelines on how new code should be written for inclusion in scikit-learn, and which may be appropriate to adopt in external projects."

jnothman · 2019-08-13T10:28:41Z

doc/developers/developing_scikit.rst

+
+.. _coding-guidelines:
+
+Coding guidelines


I think this belongs below "Rolling your own estimator"

jnothman · 2019-08-13T10:28:46Z

doc/developers/developing_scikit.rst

+  find bugs in scikit-learn.
+
+* Use the `numpy docstring standard
+  <https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt>`_


This should be updated: https://numpy.readthedocs.io/en/latest/format.html

jnothman · 2019-08-13T10:28:47Z

doc/developers/developing_scikit.rst

+
+.. _testing_coverage:
+
+Testing and improving test coverage


I'm now thinking this belongs back in the contributing guide. It's more specific to scikit-learn core than I had realised. (It would be nice to eventually include some general comments here on how to test estimators.)

jnothman · 2019-08-13T10:28:48Z

doc/developers/developing_scikit.rst

+
+* **Please don't use** ``import *`` **in any case**. It is considered harmful
+  by the `official Python recommendations
+  <https://docs.python.org/2/howto/doanddont.html#from-module-import>`_.


This URL should be updated: https://docs.python.org/3.1/howto/doanddont.html#at-module-level

@jnothman Thanks for the feedback. I made all the changes you recommended above.

arpanchowdhry · 2019-08-16T22:40:49Z

@jnothman Let me know if you have any other feedback.

jnothman

The Coding Guidelines section is still somewhat duplicated between the two sections. The sections on "Deprecation" and "Change the default value of a parameter" could be under a new heading called "Maintaining backwards compatibility" instead of the Coding Guidelines. I don't think "
Python versions supported" needs to be in this file.

doc/developers/developing_scikit.rst

jnothman · 2019-08-18T11:21:52Z

doc/developers/developing_scikit.rst

@@ -0,0 +1,671 @@
+.. _developing_scikit:


We're going to need to give this a better filename. I'm not sure yet what's right. coding? implementation? api?

Personally I like 'developing' more than 'coding', but I will rely on your judgement.

"Developing" wrt an OSS project often includes project & issue management, personnel, etc. So it covers "contributing" as well as this. Rather this section is a guide to engineering, etc.

Grasping at straws now, but maybe use 'enhancement' or 'enhancing_scikit'. That word is used heavily at my day job :)

This is not about enhancing. This is about how to implement/develop/code/build your own estimators. The filename should be a single word, not two. I'm okay with you choosing one of those words. Thanks

jnothman · 2019-08-18T11:26:03Z

doc/developers/developing_scikit.rst

+==================================
+Developing scikit-learn estimators
+==================================
+


Insert .. currentmodule:: sklearn

@jnotham I am not sure why but I am getting build failures with '.. currentmodule:: sklearn'. But once I removed it all the failures are gone.

arpanchowdhry · 2019-08-19T05:58:09Z

The Coding Guidelines section is still somewhat duplicated between the two sections. The sections on "Deprecation" and "Change the default value of a parameter" could be under a new heading called "Maintaining backwards compatibility" instead of the Coding Guidelines. I don't think "
Python versions supported" needs to be in this file.

@jnothman I can take out 'Coding Guidelines' from one of the files in you would like, but seems useful in both places though.

jnothman · 2019-08-19T06:01:02Z

I think we should make it clear that people reading the Contributing guide should also read the other section.

jnothman · 2019-08-20T08:13:11Z

Those build failures were not due to any of your changes, but due to failures elsewhere that we subsequently fixed.

jnothman

I'm happy to approve this apart from that filename.

jnothman · 2019-08-21T10:46:28Z

doc/developers/developing_scikit.rst

@@ -0,0 +1,671 @@
+.. _developing_scikit:


This is not about enhancing. This is about how to implement/develop/code/build your own estimators. The filename should be a single word, not two. I'm okay with you choosing one of those words. Thanks

arpanchowdhry · 2019-08-22T22:57:15Z

@jnothman Thanks for the feedback. I changed the filename to develop.

jnothman

Seems a big improvement to me.

jnothman · 2019-08-26T23:45:27Z

Could we get another pair of eyes on this and merge?

thomasjpfan

This is great @arpanchowdhry !

NicolasHug · 2019-08-27T12:25:30Z

Thanks for the work @arpanchowdhry!

@jnothman @thomasjpfan sorry I wasn't able to review earlier. I feel that the Plotting API section is in an awkward position now. Maybe we can move it to the Developing scikit-learn estimators section if we rename this one to something else?

thomasjpfan · 2019-08-27T12:49:46Z

Both sections are kind of awkward for Plotting API to be in. I considered “contributing” = “anything that is not about developing an estimator”.

Developing scikit-learn estimators is really well targeted to third party libraries. Adding the plotting to the end seems a little strange, even with a change in title.

Plotting API most likely needs its own section, considering with PDPs we have space stealing.

jnothman · 2019-08-27T12:49:58Z

I thought it was about this specific library, for now??? No strong opinion.

NicolasHug · 2019-08-27T13:25:41Z

I thought too, especially considering we have coding style recommendation

thomasjpfan · 2019-08-27T13:33:18Z

After describing what is an estimator, it goes into “rolling your own estimator”. Then estimator tags. These topics are very generic about “how to create an estimator” Finally, at the end it goes into sklearn specific “coding guidelines”.

thomasjpfan · 2019-08-27T13:35:11Z

It is also well targeted for this specific library as well. ;)

arpanchowdhry added 3 commits August 8, 2019 23:00

Separating documentation for contributing and developing estimators i…

a97a12f

…n to different files

Updated index.rst to reflect the new developing estimators file

200fc5b

Updated index.rst not to reflect the new developing estimators file

15f0f5e

NicolasHug reviewed Aug 9, 2019

View reviewed changes

arpanchowdhry added 2 commits August 10, 2019 11:01

Change file name/ref etc to remove estimators and add scikit, other m…

a3aebb0

…inor changes

Minor change to to header

f1fcb4a

jnothman reviewed Aug 13, 2019

View reviewed changes

Improvments based on feedback from jnothman

d9fa05a

jnothman reviewed Aug 13, 2019

View reviewed changes

Merged changes from scikit-learn#14545 and further improvements

47d24af

jnothman reviewed Aug 18, 2019

View reviewed changes

Some more documentation improvements

b01df46

arpanchowdhry added 2 commits August 18, 2019 23:12

Documentation improvements

3e89ec3

Removing

69043bb

arpanchowdhry added 2 commits August 20, 2019 16:52

Merging scikit-learn#14680

21cee3c

Adding .. currentmodule:: sklearn

9a5e0ad

jnothman reviewed Aug 21, 2019

View reviewed changes

arpanchowdhry added 2 commits August 22, 2019 15:48

remove developing_scikit.rst

0524af3

changed filename to develop.rst

63a4a10

jnothman approved these changes Aug 24, 2019

View reviewed changes

Changed index.rst to reflect new filename

06eddd0

jnothman added the Documentation label Aug 26, 2019

jnothman added High Priority High priority issues and pull requests Waiting for Reviewer labels Aug 26, 2019

thomasjpfan approved these changes Aug 27, 2019

View reviewed changes

thomasjpfan merged commit dc11759 into scikit-learn:master Aug 27, 2019

Uh oh!

Conversation

arpanchowdhry commented Aug 9, 2019

Uh oh!

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

arpanchowdhry commented Aug 10, 2019

Uh oh!

NicolasHug commented Aug 12, 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!

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!

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!

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!

arpanchowdhry commented Aug 16, 2019

Uh oh!

jnothman left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

arpanchowdhry commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman commented Aug 19, 2019 via email

Uh oh!

jnothman commented Aug 20, 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!

arpanchowdhry commented Aug 22, 2019

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

arpanchowdhry commented Aug 19, 2019 •

edited

Loading