Skip to content

DOC corrected docstring on make_classification#22797

Merged
thomasjpfan merged 4 commits intoscikit-learn:mainfrom
DeaMariaLeon:docstring1
Mar 14, 2022
Merged

DOC corrected docstring on make_classification#22797
thomasjpfan merged 4 commits intoscikit-learn:mainfrom
DeaMariaLeon:docstring1

Conversation

@DeaMariaLeon
Copy link
Copy Markdown
Member

Reference Issues/PRs

Addresses meta issue #21350

What does this implement/fix? Explain your changes.

Corrected docstring on make_classification

Any other comments?

#pariswimlds

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.

Thank you for the PR!


y : ndarray of shape (n_samples,)
The integer labels for class membership of each sample.

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.

To fix the linting issues:

Suggested change

--------
make_blobs : Simplified variant.
make_multilabel_classification : Unrelated generator for multilabel tasks.
selection benchmark", 2003.
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.

Same here regarding linting issues:

Suggested change
selection benchmark", 2003.
selection benchmark", 2003.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi, thanks for the review... I corrected the error on _samples_generator.py, but I'm not sure how to proceed with the PR. Do I close it and start all over with a new PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Answering my own question: I think I have to push to my fork, and this will update the PR. In case this is not correct, please let me know. This is my first PR ever, so thank you for your patience.

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.

That is correct. Pushing to the branch on your fork will update the PR.

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.

May you remove this line:

"sklearn.datasets._samples_generator.make_classification",

so that the CI runs the numpydoc tests on the make_classification?

@DeaMariaLeon
Copy link
Copy Markdown
Member Author

I think I should have done a pull before the last push. But now I'm not sure if that will fix it. Sorry

@thomasjpfan
Copy link
Copy Markdown
Member

I think I should have done a pull before the last push. But now I'm not sure if that will fix it. Sorry

Since many people were working on test_docstrings.py, there is a higher change to have a merge conflict. Assuming you have configured upstream by running the following:

git remote add upstream git@github.com:scikit-learn/scikit-learn.git

You can try to fix the merge conflict by running:

git fetch upstream

# make sure you are on the `docstring1` branch
git checkout docstring1

# Merge `upstream/main` into your branch.
git merge upstream/main

There will be a merge conflict you would need to resolve.

@DeaMariaLeon
Copy link
Copy Markdown
Member Author

There will be a merge conflict you would need to resolve.

Thank you for showing me the steps to try. I don't know why it is skipping tests now.

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.

LGTM

@thomasjpfan
Copy link
Copy Markdown
Member

Thank you for showing me the steps to try. I don't know why it is skipping tests now.

I tested your PR locally and can confirm that make_classification is being validated in tests. I'll merge this PR. Thank you for working on this issue!

@thomasjpfan thomasjpfan merged commit 322e92d into scikit-learn:main Mar 14, 2022
@DeaMariaLeon DeaMariaLeon deleted the docstring1 branch March 14, 2022 15:09
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants