Skip to content

DOC Add links to pipelines examples in docstrings #26904

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

Merged
merged 8 commits into from
Aug 21, 2023

Conversation

StefanieSenger
Copy link
Contributor

This PR suggests to add links to the examples from the Pipelines and composite estimators section in the examples to the user guide and to docstrings of the respective classes and functions.

@github-actions
Copy link

github-actions bot commented Jul 26, 2023

❌ Linting issues

This PR is introducing linting issues. Here's a summary of the issues. Note that you can avoid having linting issues by enabling pre-commit hooks. Instructions to enable them can be found here.

You can see the details of the linting issues under the lint job here


ruff

ruff detected issues. Please run ruff --fix --show-source . locally, fix the remaining issues, and push the changes. Here you can see the detected issues. Note that the installed ruff version is ruff=0.0.285.


sklearn/utils/tests/test_utils.py:528:12: E721 Do not compare types, use `isinstance()`
    |
527 |     assert a_s == ["c", "b", "a"]
528 |     assert type(a_s) == list
    |            ^^^^^^^^^^^^^^^^^ E721
529 | 
530 |     assert_array_equal(b_s, ["c", "b", "a"])
    |

sklearn/utils/tests/test_utils.py:534:12: E721 Do not compare types, use `isinstance()`
    |
533 |     assert c_s == [3, 2, 1]
534 |     assert type(c_s) == list
    |            ^^^^^^^^^^^^^^^^^ E721
535 | 
536 |     assert_array_equal(d_s, np.array([["c", 2], ["b", 1], ["a", 0]], dtype=object))
    |

Found 2 errors.

Generated for commit: 41d2357. Link to the linter CI: here

@adrinjalali
Copy link
Member

Could you please add in this issue (#26927) a comment with a link to the PR for each PR of this type and the examples they're adding to the docs?

@StefanieSenger
Copy link
Contributor Author

@glemaitre

@@ -69,6 +69,11 @@ class Pipeline(_BaseComposition):
to another estimator, or a transformer removed by setting it to
`'passthrough'` or `None`.

For examples use cases of `Pipeline` combined with
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For examples use cases of `Pipeline` combined with
For use case examples of :class:`Pipeline` combined with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also here, the link would link to the currently open document.

@@ -69,6 +69,11 @@ class Pipeline(_BaseComposition):
to another estimator, or a transformer removed by setting it to
`'passthrough'` or `None`.

For examples use cases of `Pipeline` combined with
Copy link
Member

Choose a reason for hiding this comment

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

I would split this into two sentences.

The first sentence gives an example of the simplest pipeline use-case: pipelining estimators (PCA + learner). The second sentence illustrates how to optimize the hyperparameters of estimators within a pipeline.

I think the second example could be added directly in the discussion above that explains the __ story because it is precisely what you need to know for the GridSearchCV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I have separates those. Though I left the second sentence below the first one instead of pulling it up, because it is indeed the more complex example.

@adrinjalali
Copy link
Member

Updating with upstream/main should fix the linting issue

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

In other PRs, we've moved the cases where it's mostly just "For a more detailed example see ..." to the "Examples" section. Otherwise LGTM.

StefanieSenger and others added 2 commits August 18, 2023 13:35
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
@StefanieSenger
Copy link
Contributor Author

Thanks @adrinjalali, I have evaluated the positions of the 0815 example links and moved some to the "Examples" section.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM. The linter issue is fixed if you merge with main.

@glemaitre glemaitre merged commit 401c917 into scikit-learn:main Aug 21, 2023
@glemaitre
Copy link
Member

Thanks @StefanieSenger LGTM on my side.

TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
@StefanieSenger StefanieSenger deleted the link_examples_pipelines branch August 23, 2023 11:15
akaashpatelmns pushed a commit to akaashp2000/scikit-learn that referenced this pull request Aug 25, 2023
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
jeremiedbb pushed a commit that referenced this pull request Sep 20, 2023
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
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.

3 participants