Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Make slowness warning for legend(loc="best") more accurate.#14894
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
... by actually timing the call duration. Locally I can best-locatelegends even with plots with hundreds of thousands of points basicallyinstantly, so the old warning was spurious.The new test is obviously a bit brittle because it depends on how fastthe machine running it is. It's also slower than the test before(intentionally, because now you *actually* need a slow-to-place legendto trigger the warning).The warning is only emitted after the legend has been placed, but thatseems fine -- if the best-placement is so slow that you ctrl-c theprocess, you'll have a traceback anyways. Also, spawning a separatethread to always emit the warning after exactly 5s will likely just makethings worse performance-wise on average.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
include the actual run time? Wish we could use the walrus here ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fair enough.
seems like a good idea |
... 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 youactually 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