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.

Sign up for GitHub

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

Jump to bottom

ENH: Allow performance warnings to be disabled #56921

Merged
merged 11 commits into from
Feb 23, 2024
Merged

ENH: Allow performance warnings to be disabled #56921

merged 11 commits into from
Feb 23, 2024

Conversation

rhshadrach
Copy link
Member

@rhshadrach rhshadrach commented Jan 17, 2024 •
edited
Loading

This still needs tests - I plan to add tests for disabling the warnings wherever we currently hit a PerformanceWarning in the test suite. But I'd like to get feedback on the linked issue first.

Tests added.

@rhshadrach rhshadrach added Enhancement Warnings Warnings that appear or should be added to pandas labels Jan 17, 2024
@rhshadrach rhshadrach mentioned this pull request Jan 17, 2024
Closed
@rhshadrach
Merge remote-tracking branch 'upstream/main' into enh_options_perf_wa…
97c5a1c 
…rning
@rhshadrach
Add tests
1077112
@rhshadrach
Merge branch 'enh_options_perf_warning' of https://github.com/rhshadr…
56c149d 
…ach/pandas into enh_options_perf_warning

� Conflicts:
�	doc/source/whatsnew/v3.0.0.rst
�	pandas/core/config_init.py
�	pandas/core/indexes/multi.py
�	pandas/core/internals/managers.py
�	pandas/core/reshape/reshape.py
�	pandas/io/pytables.py
@rhshadrach
Merge branch 'main' of https://github.com/pandas-dev/pandas into enh_…
cd8e1ee 
…options_perf_warning

� Conflicts:
�	doc/source/whatsnew/v3.0.0.rst
�	pandas/core/internals/managers.py
�	pandas/tests/frame/indexing/test_indexing.py
�	pandas/tests/frame/indexing/test_insert.py
�	pandas/tests/frame/test_block_internals.py
@rhshadrach
Merge remote-tracking branch 'upstream/main' into enh_options_perf_wa…
1bbca5b 
…rning
@rhshadrach
fixup
f0de9be
@rhshadrach
Copy link
Member Author

For the test fixture, I wanted to be able to pass it directly to tm.assert_produces_warning instead of having the fixture return True/False and then have lines like

warn = PerformanceWarning if uses_performance_warning else False
with tm.assert_produces_warning(warn):
    ...

@rhshadrach rhshadrach marked this pull request as ready for review February 8, 2024 03:02
mroeschke
mroeschke reviewed Feb 8, 2024
View reviewed changes
pandas/core/arrays/arrow/_arrow_utils.py
msg = "Falling back on a non-pyarrow code path which may decrease performance."
if version is not None:
msg += f" Upgrade to pyarrow >={version} to possibly suppress this warning."
warnings.warn(msg, PerformanceWarning, stacklevel=find_stack_level())
Copy link
Member

@mroeschke mroeschke Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Going forward how can we ensure that PerformanceWarnings are not issued without checking the option?

Copy link
Member Author

@rhshadrach rhshadrach Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We currently fail if a warning is emitted in a test outside of tm.assert_produces_warning.

Copy link
Member

Choose a reason for hiding this comment

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

True, as long as we have a test for it :)

mroeschke
mroeschke approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

LGTM

@rhshadrach
Merge remote-tracking branch 'upstream/main' into enh_options_perf_wa…
32d1b23 
…rning

# Conflicts:
#	pandas/core/internals/managers.py
@rhshadrach
Merge remote-tracking branch 'upstream/main' into enh_options_perf_wa…
c648422 
…rning

# Conflicts:
#	doc/source/whatsnew/v3.0.0.rst
@rhshadrach
Merge remote-tracking branch 'upstream/main' into enh_options_perf_wa…
817a4b6 
…rning

# Conflicts:
#	pandas/conftest.py
@rhshadrach
Copy link
Member Author

@mroeschke - should be good to merge.

@mroeschke mroeschke added this to the 3.0 milestone Feb 23, 2024
@mroeschke mroeschke merged commit a730486 into pandas-dev:main Feb 23, 2024
47 checks passed
@mroeschke
Copy link
Member

Thanks @rhshadrach

@rhshadrach rhshadrach deleted the enh_options_perf_warning branch February 23, 2024 22:51
@DeaMariaLeon DeaMariaLeon mentioned this pull request Feb 27, 2024
Open
@jorisvandenbossche jorisvandenbossche mentioned this pull request May 5, 2024
Closed
@jorisvandenbossche
Copy link
Member

Late feedback, but was thinking about this in the context of #58581, but it would make sense to change the default to something like None? This default would mean that users see those performance warnings that we decided to turn on by default. More specifically, that would allow us to add new performance warnings that are not shown by default (in contrast to existing ones), while still giving users a way to turn them on.

@rhshadrach
Copy link
Member Author

rhshadrach commented May 5, 2024 •
edited
Loading

That behavior sounds like an improvement to me, but I'd still prefer #55385 over it. Maybe discuss this alternative there? Would be good to have a list of performance warnings we'd want on by default (I'd guess we'd start with whatever warnings we have now, but that might not be the same as want we want to end up with).

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
@rhshadrach @pmhatre1
ENH: Allow performance warnings to be disabled (pandas-dev#56921)
660f2ed 
* ENH: Allow performance warnings to be disabled

* ENH: Allow performance warnings to be disabled

* Add tests

* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroeschke mroeschke mroeschke approved these changes

@jorisvandenbossche jorisvandenbossche Awaiting requested review from jorisvandenbossche

Assignees
No one assigned
Labels
Enhancement Warnings Warnings that appear or should be added to pandas
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

ENH: Allow users to disable PerformanceWarning
3 participants
@rhshadrach @mroeschke @jorisvandenbossche