Repository navigation
Improve rendering when using multiple / large labels - #21958
ojeda-e wants to merge 5 commits into
Conversation
Sorry, something went wrong.
|
This seems reasonable. Unfortunately, the figure doesn't survive a pickle - i.e. with the new cache, the objects are not serializable. I expect the figure used to have something that detached the cache, and you could likely do the same thing here? But I'm not an expert on (nor a fan of) pickling figures ;-) |
Sorry, something went wrong.
|
Thanks for the brief explanation @jklymak. |
Sorry, something went wrong.
|
Many tests fail locally for me as well. If you are pretty sure it's not due to your pr it's perfectly fine to push tothe more controlled test environment. |
Sorry, something went wrong.
|
Thanks for the comments @tacaswell, I hope I addressed your comments with the recent changes. |
Sorry, something went wrong.
|
I wonder(?) if it may make sense to keep the old global cache layer as well, so that if you're e.g. drawing 10 axes each with the same tick labels, the labels on the latter axes still benefit from the work done on the first axes. Of course, that would require a bit more rearchitecting of the caching... |
Sorry, something went wrong.
| """Handle storing and drawing of text in window or data coordinates.""" | ||
|
|
||
| zorder = 3 | ||
| _cached = cbook.maxdict(50) |
There was a problem hiding this comment.
Is this dict really a memory hog if it gets larger? Why not just set to a large number and move on? Just because we can make a double caching structure doesn't mean we should. Given that we have had very few complaints of this nature, my assumption is that most people are not hitting the 50-element limit, so bumping it to 5000 for the few people who need it doesn't seem like a terrible idea.
Note that this largely catches users who have accidentally used categoricals and created thousands of ticks. I think, if anything, we should add logic in the categorical ticker that raises a warning of more than 100 ticks are being asked for.
Sorry, something went wrong.
Sorry, something went wrong.
|
No, because then there was #22323 which further got rid of the explicit _cached in favor of a lru_cache. There may(?) still be some small benefit to caching layout info per-instance, although restoring that (which was mostly removed in #22271, which places the cache a bit earlier in the call tree) would require some more work, not a simple rebase (and I don't know if timings would show a large further improvement). |
Sorry, something went wrong.
|
Moved to 3.8 as the rebase is non trivial and there have been other changes to this caching layer so we need to evaluate if we still need this. |
Sorry, something went wrong.
|
Original issue is claimed to be fixed by #22271. |
Sorry, something went wrong.
PR Summary
Fixes #21895
The example reported in #21895 initially runs in 10.079 s (locally, my old laptop). By moving cache in lib/matplotlib/text.py as an attribute of Text, the example runs in 4.426 s, which translates to an improvement of ~78%. Added a unit test that uses labels of different lengths, increasing as 10**i, and assert times are shorter. Not quite as the suggestion by @tacaswell because I am not comparing text vs no text. I'm not fully convinced of this approach either, but can't think of anything better. I appreciate suggestions to make the test more solid.
No changes in matplotlib/lib/matplotlib/mathtext.py.
As a separate note, this also probably needs a benchmark in mpl_benchmarks as suggested by @jklymak on Gitter (thanks Jody!).
PR Checklist
Tests and Styling
Documentation