Skip to content

DOC Removing warnings from plot CV examples #29072

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 2 commits into from
May 22, 2024

Conversation

ojoaomorais
Copy link
Contributor

Reference Issues/PRs

#29055

What does this implement/fix? Explain your changes.

Removing warnings from plot CV examples

Copy link

github-actions bot commented May 21, 2024

✔️ Linting Passed

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

Generated for commit: bf10111. Link to the linter CI: here

import matplotlib.pyplot as plt
import numpy as np
from matplotlib.patches import Patch

# Removing warnings from examples
warnings.filterwarnings("ignore")
Copy link
Member

Choose a reason for hiding this comment

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

I think this removes all warnings. I would prefer that we only remove UserWarnings.

@ArturoAmorQ
Copy link
Member

ArturoAmorQ commented May 22, 2024

Filtering warnings should be the last of the options. In this case I was thinking more of modifying the plot_cv_indices function to something like this:

    use_groups = 'Group' in type(cv).__name__
    groups = group if use_groups else None

    # Generate the training/testing visualizations for each CV split
    for ii, (tr, tt) in enumerate(cv.split(X=X, y=y, groups=groups)):
    ...

@ojoaomorais
Copy link
Contributor Author

Filtering warnings should be the last of the options. In this case I was thinking more of modifying the plot_cv_indices function to something like this:

    use_groups = 'Group' in type(cv).__name__
    groups = group if use_groups else None

    # Generate the training/testing visualizations for each CV split
    for ii, (tr, tt) in enumerate(cv.split(X=X, y=y, groups=groups)):
    ...

Thanks for the feedback, @ArturoAmorQ. If I may ask, why is removing all the warnings from this doc page not a good idea?

@ArturoAmorQ
Copy link
Member

If I may ask, why is removing all the warnings from this doc page not a good idea?

Because of several possibilities:

  • in the future we may have new warnings coming from other lines of code that we don't want to overlook, for instance, coming from deprecations in other libraries;
  • if we silence them in the whole example, another contributor may change code that would otherwise rise a warning for a good reason;
  • simply because warnings are sometimes introduced for enforcing good practices, by solving them, we adopt such good practices.

Copy link
Member

@ArturoAmorQ ArturoAmorQ left a comment

Choose a reason for hiding this comment

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

In any case, this PR looks good to me as it is. Thanks for your contribution @ojoaomorais!

@ArturoAmorQ ArturoAmorQ enabled auto-merge (squash) May 22, 2024 12:27
@ArturoAmorQ ArturoAmorQ merged commit 78f0c00 into scikit-learn:main May 22, 2024
28 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 2, 2024
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
11 tasks
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