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 linked SplineTransformer to time-related feature engineering example #26971

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Conversation

scaja
Copy link
Contributor

@scaja scaja commented Aug 1, 2023

Reference Issues/PRs

issue #26927

What does this implement/fix? Explain your changes.

This PR linked SplineTransformer to time-related feature engineering example

Related Example files:

plot_cyclical_feature_engineering.py

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 8678ea4. Link to the linter CI: here

@scaja scaja changed the title linked SplineTransformer to time-related feature engineering example DOC linked SplineTransformer to time-related feature engineering example Aug 1, 2023
Comment on lines +587 to +588
In order to learn more about the SplineTransformer class go to:
:ref:`sphx_glr_auto_examples_applications_plot_cyclical_feature_engineering.py`
Copy link
Member

Choose a reason for hiding this comment

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

This is a good example, and could be mentioned in more places since it's more on the realistic side of things.

I'd suggest having a link in ColumnTransformer, Pipeline, and TimeSeriesSplit as well.

But the description could be more informative. Something like:

Fore a more comprehensive example on a time series data est, see ...

WDYT @glemaitre

Copy link
Member

Choose a reason for hiding this comment

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

Definitely for TimeSeriesSplit. For ColumnTransformer and Pipeline, let's do it as well. We have to be careful in the future that for those two, we don't have too many examples.

@manthanindane
Copy link

I'm new here and figuring out some things.......Is this issue still open? Is anybody working on this?

@glemaitre
Copy link
Member

@manthanindane The corresponding issue is still open here: #26927

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

+1 for merging this PR as such as it's already a net improvement.

@adrinjalali
Copy link
Member

Added the link for TimeSeriesSplit, leaving the other ones alone. Feel free to merge @ogrisel or @glemaitre

@glemaitre glemaitre enabled auto-merge (squash) December 4, 2023 15:22
@glemaitre glemaitre merged commit f113327 into scikit-learn:main Dec 4, 2023
25 checks passed
@Ujjwal29
Copy link

Ujjwal29 commented Apr 3, 2024

How do we figure out which example file reference to be added in which file?

@adrinjalali
Copy link
Member

@Ujjwal29 it's either in the docstrings of related classes (inside the .py files where they're implemented) or in the user guides (the .rst files where the user guides are written)

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.

6 participants