Skip to content

DOC Add links to pipelines examples in docstrings#26904

Merged
glemaitre merged 8 commits intoscikit-learn:mainfrom
StefanieSenger:link_examples_pipelines
Aug 21, 2023
Merged

DOC Add links to pipelines examples in docstrings#26904
glemaitre merged 8 commits intoscikit-learn:mainfrom
StefanieSenger:link_examples_pipelines

Conversation

@StefanieSenger
Copy link
Copy Markdown
Member

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
Copy Markdown

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.

Details

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
Copy Markdown
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
Copy Markdown
Member Author

@glemaitre

to another estimator, or a transformer removed by setting it to
`'passthrough'` or `None`.

For examples use cases of `Pipeline` combined with
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.

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

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.

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

to another estimator, or a transformer removed by setting it to
`'passthrough'` or `None`.

For examples use cases of `Pipeline` combined with
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.

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
Copy Markdown
Member 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
Copy Markdown
Member

Updating with upstream/main should fix the linting issue

Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

Copy link
Copy Markdown
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
Copy Markdown
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