Make slowness warning for legend(loc="best") more accurate. #14894
+22
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tacaswell
reviewed
Jul 26, 2019
... by actually timing the call duration. Locally I can best-locate legends even with plots with hundreds of thousands of points basically instantly, so the old warning was spurious. The new test is obviously a bit brittle because it depends on how fast the machine running it is. It's also slower than the test before (intentionally, because now you *actually* need a slow-to-place legend to trigger the warning). The warning is only emitted after the legend has been placed, but that seems fine -- if the best-placement is so slow that you ctrl-c the process, you'll have a traceback anyways. Also, spawning a separate thread to always emit the warning after exactly 5s will likely just make things worse performance-wise on average.
tacaswell
reviewed
Jul 26, 2019
|
|
||
| if self._loc_used_default and time.perf_counter() - start_time > 1: | ||
| cbook._warn_external( | ||
| 'Creating legend with loc="best" can be slow with large ' |
There was a problem hiding this comment.
include the actual run time? Wish we could use the walrus here ;)
There was a problem hiding this comment.
would rather not, as if we do I'm fairly worried the next thing is that someone will ask for the warning threshold to be configurable and whatnot.
tacaswell
approved these changes
Jul 26, 2019
jklymak
approved these changes
Jul 26, 2019
|
seems like a good idea |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
... by actually timing the call duration. Locally I can best-locate
legends even with plots with hundreds of thousands of points basically
instantly, so the old warning was spurious.
The new test is obviously a bit brittle because it depends on how fast
the machine running it is. It's also slower than the test before
(intentionally, because now you actually need a slow-to-place legend
to trigger the warning).
The warning is only emitted after the legend has been placed, but that
seems fine -- if the best-placement is so slow that you ctrl-c the
process, you'll have a traceback anyways. Also, spawning a separate
thread to always emit the warning after exactly 5s will likely just make
things worse performance-wise on average.
The 5s delay is the same as used in font_manager.py.
The original warning went in as #12455.
PR Summary
PR Checklist