The Wayback Machine - https://web.archive.org/web/20220305073009/https://github.com/scikit-learn/scikit-learn/pull/22401
Skip to content
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 Add minimal reproducer guide for scikit-learn #22401

Merged
merged 31 commits into from Mar 3, 2022

Conversation

ArturoAmorQ
Copy link
Contributor

@ArturoAmorQ ArturoAmorQ commented Feb 7, 2022

Reference Issues/PRs

Fixes #21945.

What does this implement/fix? Explain your changes.

This PR adds a guide to the online documentation on how to craft a minimal reproducer oriented to bug report in scikit-learn. In particular it:

  • Explains by giving short code examples on how to generate small synthetic datasets with the numpy random module, pandas, sklearn.dataset.make_classification, make_regression, make_blobs, or any of the scikit-learn loading dataset utilities;
  • Explains step by step how to simplify the code and hyperparameters to find a minimal model that can reproduce the non-expected behavior;
  • Explains that the effort made to craft a minimal reproducible code example will help remove any ambiguity in the English written description in the bug report and allow maintainers or other contributors to find a fix for the problem efficiently;
  • Explains the markdown formatting for triple backticks with python or python-traceback qualifiers;
  • Links this document to the contributing page;
  • Adds the document to the index.

Any other comments?

This is still WIP, so any feedback on how to improve this document is always welcomed.

Copy link
Member

@jmloyola jmloyola left a comment

Thanks @ArturoAmorQ.
This LGTM!

I left a couple of comments, nitpicks mostly.

doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
@jmloyola
Copy link
Member

@jmloyola jmloyola commented Feb 9, 2022

We could also add a link to this guide in the bug report template.

Co-authored-by: Juan Martin Loyola <[email protected]>
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
@ArturoAmorQ ArturoAmorQ changed the title [WIP] DOC Add minimal reproducer guide for scikit-learn DOC Add minimal reproducer guide for scikit-learn Feb 17, 2022
Copy link
Member

@cmarmo cmarmo left a comment

Hi @ArturoAmorQ , this is a very useful addition.
The sphinx warning can be avoided adding a blank line before the itemizing.

doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
Copy link
Member

@jjerphan jjerphan left a comment

LGTM up to some minor fixes. Thank you, @ArturoAmorQ!

doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
Co-authored-by: Julien Jerphanion <[email protected]>
@ArturoAmorQ
Copy link
Contributor Author

@ArturoAmorQ ArturoAmorQ commented Feb 21, 2022

Thank you for the comments, @jmloyola, @cmarmo and @jjerphan !

Copy link
Member

@ogrisel ogrisel left a comment

This looks very nice. Here is a first pass of review comments:

doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
Copy link
Member

@ogrisel ogrisel left a comment

To better get a feeling of what we mean by minimal, let's actually show how to simplify an overly complex code snippet that we typically get as feedback.

doc/developers/minimal_reproducer.rst Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Show resolved Hide resolved
ogrisel
ogrisel approved these changes Mar 2, 2022
Copy link
Member

@ogrisel ogrisel left a comment

Some more minor comments, other than that, LGTM!

doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Outdated Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Show resolved Hide resolved
doc/developers/minimal_reproducer.rst Show resolved Hide resolved
@jjerphan jjerphan merged commit e107e61 into scikit-learn:main Mar 3, 2022
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants