-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Warn when "best" loc of legend is used with lots of data #12455
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
API wise, I think if the user has not specified |
Interesting idea. Would you still warn if they hardwired |
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... |
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.
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). |
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. |
I actually disagree, I strongly think we should treat users as adults and assume that if they explicitly write 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. |
Ok, so decisions on what should be done here? |
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. |
fwiw I actually prefer 'best' as default (and warn only if loc is None). |
I think I will plan on leaving the default as I do think it will be useful to only trigger the warning only when |
Added some commits that accomplish the following:
|
A legend with a `np.random.random(500000)` array plotted was a real pain to interact with.
I updated the N required in code but not in test.
8472e45
to
504f619
Compare
Heads up, had to rebase to account for changes made in #12006 |
copy/pasted but didn't get rid of non-applicable comment
Co-Authored-By: kbrose <kevin+gh@maypark.com>
Co-Authored-By: kbrose <kevin+gh@maypark.com>
bc91f46
to
f89a78c
Compare
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.
Thanks @kbrose ! Sorry it took so long to merge it
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