Warn when "best" loc of legend is used with lots of data

[kbrose]

PR Summary

Warn when "best" loc of legend is used with lots of data, as it can be quite slow.

Ref #12120 and #12203

PR Checklist

• Has Pytest style unit tests
• Code is Flake 8 compliant
• N/A New features are documented, with examples if plot related
• N/A Documentation is sphinx and numpydoc compliant
• N/A Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
• N/A Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

[jklymak]

API wise, I think if the user has not specified loc='best', (i.e. loc is None) and this case comes up, we should warn and set loc=1' to draw the legend quickly. Of course, if they have hardwired loc='best'` we shouldn't change that - they may be willing to wait.

[kbrose]

Interesting idea. Would you still warn if they hardwired 'best'?

[jklymak]

I don’t know that’s an interesting question. If the real problem is people using the default, then maybe the warning should just be on the default and the user input trusted if they specify directly...

[timhoffm]

I would always warn on long 'best' runs no matter if default or explicitly set. That code could be quite hidden, or people switch from using small test data to large production data and wonder why their plot suddenly takes so long. Either way, we should warn if it takes long. Users can filter the warning if they want to suppress it.

API wise, I think if the user has not specified loc='best', (i.e. loc is None) and this case comes up, we should warn and set loc=1' to draw the legend quickly.

Not sure if it's worth the extra code. Additionally, it's an API change that would have to be documented (and in theory be warned about for the deprecation period before we actually do the code change).

[kbrose]

Could potentially merge in the warning and open another PR to see what it would look like to implement that and discuss more about the API change.

[anntzer]

I actually disagree, I strongly think we should treat users as adults and assume that if they explicitly write ="best" they know what they're doing.

If they didn't put it (and inherited "best" from the rcParams), I guess a warning is fine.

You can set up a delayed warning as in https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/font_manager.py#L231 to only emit the warning if the call takes more than e.g. 1s (or whatever looks reasonable) though that may or may not be overkill here.

[kbrose]

Ok, so decisions on what should be done here?

[jklymak]

I think warn only if loc is None. I also think we should change the loc to north east or upper right or whatever it’s called. I.e. chose a reasonable default that computes reasonably fast Of course include in the warning how the user can specify best explicitly.

[anntzer]

fwiw I actually prefer 'best' as default (and warn only if loc is None).
See also #7121 (comment) for a similar-ish case where @tacaswell argued against a performance-related warning.

[kbrose]

I think I will plan on leaving the default as 'best'. Even without arguments about which is "better" it feels like a fairly big change in behavior that should go through proper deprecation, and I feel that is out of scope for what I'm trying to do here. Also, I'm wiling to concede that most people do not plot as much data as I do, and that my preferred defaults don't necessarily match the typical use case.

I do think it will be useful to only trigger the warning only when 'best' is not explicitly asked for. I will try to implement that in the next couple of days.

[kbrose]

Added some commits that accomplish the following:

1. Only issue the warning if the 'best' loc is taken by default.
2. Add test to that effect.
3. Simplify the counting that determines when to issue a warning by only looking at the number of vertices.

[kbrose]

Heads up, had to rebase to account for changes made in #12006

[NelleV]

Thanks @kbrose ! Sorry it took so long to merge it