Tests arecoming! here. I tried to usenon image ones, but to do so, I did not find anything more straightforward than relying on assertions about theBboxes of the legends... If there was something way more clever to do, I am interested.

In both tests, the leftmost subplot is designed to be used as the reference case. FYI, here are the pictures (not used to perform the tests) that I used for the two tests:


while formerly the results would have been:

Sorry, I let an F-string from former iterations in the previous commit (3.6+ ❤️)... I fixed it and rebased to squash the one character-commit.

So with this change it could place a legend inside a non-solid bar (if the bar is big enough)?

good point... (but I think being fully in a solid polygon is less annoying that crossing some boundaries)

You could check if the legend was inside any polygons as well though your second test would fail. But I don’t think most of the time you want the legend inside a drawn polygon.

How much computation time does this add? With the 'auto' behavior now the default this is a bit of a hot-path for people updating plots.

@anntzer: I added an entry inapi_changes, quite minimalistic as you suggested ;).

@tacaswell: I tried to write a benchmark scripthere. It seems to me that the performance impact is barely noticeable (between +0.9% and +1.3% of execution time in the examples of the benchmark).However TBH I know very little about profiling (and monkey-patching withmock too...), so I may have misunderstand the profiler (pprofile) output. Any advice or suggestion are welcome if there were better ways to do this kind of study 🐑.

The plots used for benchmarking look like:

• current (former?) algorithm
  

• suggested (new?) algorithm
  

and they are more intended to give hard time to thelegend locator than to be representative of real-life plots. (Well,I am not used to plot histograms with ~ 5000 bins or plots with 500Polygon patches with ~ 10 edges each.)

One can see that the behavior with the patch collection is still quite surprising (not to say wrong...), and there is something weird with the plot coordinates. (I guess that I missed sometransform trick.)

@Kojoley and@jklymak: I am still looking into the consequences of this PR in the kind of situations that you suggested!

Edit: Added some values from the benchmark results.

Putting back the [WiP] mark, as some legitimate questions about the consequences of this PR have been made and still need to be evaluated.

Personally, I think putting a legend inside an artist is an OKfallback, but only if it can't be put outside an artist. I wonder if a "better" way to go would be doing the old bounding box check first and then, only if that fails for all possible locations, do the intersecting-vertex check. I think that gets better behaviour but still preserves the spirit of this PR.

Also, looking at the above, I wonder if the check order needs to be changed. I'd far prefer the legend top center than bottom center in the first test.

@jklymak About the check order: I was having the same thought (but usually@tacaswell is a bit reluctant with API changes just for “aesthetic” reasons 😈 :P). Anyway, other things are broken/weird with the check order, for example "(centered) right" seems to be evaluated twice but not even in successive positions...

For your other idea, that is an interesting idea, I'll try to give it a shot when I have more time (likely not today...).

ping@afvincent I still think this is a great enhancement....

@jklymak I did not forget it ;). It is simply that day-job and other more natural events (Thomas Fire, I am speaking of you 😈) have required a significant share of my attention during the past few weeks... Hopefully I will have more time to take care of this (and the other PRs I opened recently) during the second half of December. If something is really urgent, I do not mind friendly take overs. But I guess that the next “deadline” was pushed to 2.2, which gives a bit of time, does it not?

Yeah, but 2.2 has over 900 issues, so we will be busy ;-)

ping@afvincent real-life bah! I still think this should go in, maybe after some mods...

@jklymak Hearing you loud and clear, and this PR is still in my on-going work list ;).

I am still trying to find a useful way to benchmark that change (IIRC@tacaswell was a bit worry about that change). Besides, there was also a comment in the thread about the fact that the default order used to explore the possible position might not be the most “intuitive” one (whatever this means :P), but I guess that we might re-evaluate it for 3.0 if that is relevant.

As someone who is slowing down drawing as fast as others are speeding it up, I disagree that perfomance is a huge issue (within reason). If a user needs performance, they shouldn't use automatic convenience functions, i.e in this case, they can specify the location kwarg. But thats just my vote; I appreciate that its also the default, so it has the potential to slow people down by default is an issue.

My only problem w/ this PR is that I think its possible to theoretically put the legend inside the data curve, and that doesn't appeal.

At the very least, this could get its own value for the location kwarg. "superauto"?

Would rather not introduce a new location keyword, keep things lean, yada yada. If it's just a 1% slowdown I wouldn't worry to much about it...

ping@afvincent - I think this one was quite close if you have time...

Annual ping@afvincent...

I took the liberty of rebasing this and think it is ready for re-review.

@jklymak Can you please make the call if this should go in or be pushed to 3.8?

I took the liberty of doing a squash merge

It took 5 years to merge this PR 🙂