Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
NelleV merged 12 commits intomatplotlib:masterfromkbrose:warn-on-legend-slowness
Jan 21, 2019

Conversation

kbrose
Copy link
Contributor

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 isFlake 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
Copy link
Member

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

anntzer reacted with thumbs up emoji

@kbrose
Copy link
ContributorAuthor

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

@jklymak
Copy link
Member

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
Copy link
Member

timhoffm commentedOct 9, 2018
edited
Loading

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
Copy link
ContributorAuthor

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
Copy link
Contributor

I actually disagree, Istrongly 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 inhttps://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
Copy link
ContributorAuthor

Ok, so decisions on what should be done here?

@tacaswelltacaswell added this to thev3.1 milestoneOct 11, 2018
@jklymak
Copy link
Member

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
Copy link
Contributor

anntzer commentedOct 11, 2018
edited
Loading

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
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

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.

@kbrosekbroseforce-pushed thewarn-on-legend-slowness branch from8472e45 to504f619CompareNovember 15, 2018 00:51
@kbrose
Copy link
ContributorAuthor

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

copy/pasted but didn't get rid of non-applicable comment
timhoffmand others added3 commitsNovember 21, 2018 17:54
Co-Authored-By: kbrose <kevin+gh@maypark.com>
Co-Authored-By: kbrose <kevin+gh@maypark.com>
@kbrosekbroseforce-pushed thewarn-on-legend-slowness branch frombc91f46 tof89a78cCompareNovember 22, 2018 00:02
Copy link
Member

@NelleVNelleV left a 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

kbrose reacted with thumbs up emoji
@NelleVNelleV merged commit5702999 intomatplotlib:masterJan 21, 2019
@kbrosekbrose deleted the warn-on-legend-slowness branchJanuary 21, 2019 23:24
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@NelleVNelleVNelleV approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

6 participants
@kbrose@jklymak@timhoffm@anntzer@NelleV@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp