In general I have a problem understanding this PR. Seen from the tests, this would result in the normal code

plt.plot([1], [1], 's', markersize=40., label="label")
plt.legend()

suddenly producing a huge legend in the plot.

Shouldn't it be the other way around, namely that arbitrarily large or small markers have a normalized size in the legend? At least that would actually be the desired way to create a legend - as far as I see it from a user's perspective. Or even better, to allow to choose whether markersize should be adapted or not, i.e. adding a possible markerscale option like markerscale="normalized".

I can’t tell from the images above what the change is; they look the same to me.

@ImportanceOfBeingErnest I sort of agree w your comment above, except, that’s potentially harder than what’s done here, and I imagine we can all agree that legends that overspill their borders are undesirable. But the issue w normalization of marker sizes is that they are often used to differentiate data. I.e. a marker size of 30 could get a different legend entry than a marker size of 40. So I think normalaization would not work for many folks. Some sort of shrink factor might make sense in some cases, but legends are already a mess of kwargs so I’m not really seriously suggesting that.

@jklymak The one being highlighted in the PR is just one of the old regression tests that broke due to these changes. The real changes are shown in the new tests added to test_legend.py, where the legend box is now being resized while previously the markers would go off-bounds.

it seems like we messed up with the images , changing it.

... indeed there already is a markerscale kwarg....

Here is the difference between the two images:

@jklymak My point is that this PR seems to automatically change the legend size depending on the size of the markers. This is often not desired. Suppose e.g. having two subplots both with a legend and suddenly one of those legends is bigger - this would only be acceptable if an option exists to revert that and have both legends the same size independent on the markersize.

On the other hand we currently do have the option to change the line height etc in the legend, so using large markers would just require to tweak those parameters a bit.

If I set a certain labelspacing I want the legend to have this labelspacing. And not change it to something else just because the legend thinks that's better. Also if I use a style sheet and set the labelspacing there, I do not want some algorithm changing that.

So would it maybe make sense to have labelspacing=“auto” that would trigger this behaviour?

Yes, labelspacing="auto" would be an option. But what would then be the default value for "normal" labelspacing, which is currently set to 0.5?

The fix is not affecting the labelspacing. It is changing the size of the DrawingArea the handles are drawn in. In the test that has been changed, the legend looks like there is more labelspacing because in the original version, the artists were spilling over into the padding between DrawingAreas.

Currently, setting handlelength and handleheight can be used to grow the DrawingArea. I can add a normalize flag and add code for scaling down artists but how should I handle that? If there is a size 50 marker and a size 10 marker should they both be scaled down by the same factor or should they be scaled independently to both fit the DrawingAreas?

There needs to be the option to have the same distance between legend labels, independent on how large the marker size is. So for a given handleheight and labelspacing (and fontsize) this should not change. This seems to not be the case with this PR.

Btw. handleheight seems to be missing in the docs.

Downscaling the markers can currently be done using markerscale , which is proportional to the original marker. My proposal for normalizing would mean to have a constant markersize, independent on the one of the original artist - but that was just a wishlist item, not sure if it should be added here. Also this can be achieved using the update_function of the legend handler if needed, so it's not really urgent.

Do we actually need the additional API? Can we not infer the size of the handle (e.g., by reusing the tight_bbox code) and directly use that?
I actually (personally) have no problem with saying "you must subclass LegendHandler" (or provide all the methods that LegendHandler does, and we allow ourselves to add more) -- although you certainly need to maintain backcompat at least for a while --, but only if you can convince me that there's no other alternative.

The new tests are showing the symbol labels in the legend aligned with the bottom of the symbol. Wouldn't most people want the labels to be horizontally aligned with the center of the symbol?

In addition, I'm wondering about the cost/benefit tradeoff here. It is adding code and complexity to the already complex legend; is it really worth it?

Yes they would. I could change the alignment to center but then I would need to replace every image comparison test image with a legend it because the handles would be drawn a few pixels away from where they are now.

Given that the change to centering is required for your other change to yield the desired result, I don't see any way to avoid it. Even for the less extreme example in your PR summary, the alignment looks wrong.

I also wonder whether the default pad should be reduced.

Or, getting back to my second point in my earlier comment: how important is it to have this automatic machinery, as opposed to leaving it to the user to handle larger symbols by manually increasing the padding? Will it have unintended consequences? Will eventually handling the scatter case make the legend code even more unwieldy?

My opinion is that:

1. the code is sufficiently fenced off to not be too complicated (though it could use some documentation to be crystal clear)
2. I think its nice to have the spacing automatically adjust
3. I think if the user specified labelspacing then it should not adjust.
4. To make the failing tests pass, you could just specify the labelspacing to the current default (0.5?)

@efiring handling scatters would just require overriding the handle_width/height methods in the correct legend handler. The methods are intended to be overridden for different handle types if necessary so it's not too unwieldy.

This PR seems rather independent on labelspacing; it is rather handleheight that seems relevant here. In any case those are two parameters which can be set as arguments to the legend API, and as rcParams. Just as with any other matplotlib parameters the functionality should be (1) Use argument, if provided (2) use rcParam if argument is not provided. Hence you cannot use one or both arguments not being present in the function call as a trigger to activate this automatic sizing, as that would render the rcParams useless.

I think it is mostly agreed that the automatic change should be optional or at least allow to be turned off. The only way I see this happening is to either

• add a new argument to the legend together with the respective rcParam, something like
  adjust_to_handle_height = True (surely a better name is required); or
• let the existing argument(s) take a new value "auto"
labelspacing= "auto", handleheight = "auto"

Sorry forgot about rcParams.

I’d have rcParams set both to None and use the auto spacing be default and recover the old behaviour with the old params.

But that’s just my vote. In general I don’t think we should have overlapping text and symbols if we can help it and the resulting code isn’t ridiculously convoluted.

I can add a scaleHandleBox param. It will be true by default, It will have the current functionality while it's on. If it's off it will just go with handlelength and handleheight.

handleheight/length can't be set to auto because they're used as numbers in the normal size calculations. labelspaceing says it's "space between the legend entries" so I don't think it's relevant here.
Do you agree with this change?
P.S. is matplotlibrc.template the correct place to make changes to the rcParams?

handleheight/length can't be set to auto because they're used as numbers in the normal size calculations.

Indeed, that is basically the comment I made already, but with handleheight instead of labelspacing.

I think one would add this scalehandlebox as an argument to legend as well, right?

Since all rcParams are lower case, one would go with scalehandlebox (not sure if there isn't a better name though, but I can't think of one right now and it's probably better than adjust_to_handle_height anyways).

Adding an rcParam would be done in the lib/matplotlib/rcsetup.py file. This is the file that defines and validates the parameters. matplotlibrc.template will need to be updated as well, but that is for documentation purposes only (this file is not actually used by matplotlib itself, but should act as a valid rc file in case all parameters are uncommented). The recent PR #10306 may give you a nice overview what is needed to add an rcParam.

I added the scalehandlebox param.

I would like to change
# pack handleBox and labelBox into itemBox itemBoxes = [HPacker(pad=0, sep=self.handletextpad * fontsize, children=[h, t] if markerfirst else [t, h], align="baseline")
to
# pack handleBox and labelBox into itemBox itemBoxes = [HPacker(pad=0, sep=self.handletextpad * fontsize, children=[h, t] if markerfirst else [t, h], align="center")

(baseline to center)

It would make the legends look nicer in cases where the heights of handleBox and labelBox are different but it would require replacing the images for around 50 image comparisons.

Here's what the improvement would be:
original:

new:

The differences for most images would be of this nature:
new:

original:

diff:

Is there any reason I shouldn't make the change and update the tests?

I think the alignment change is needed, and helps even without your other changes. Maybe the thing to do is use the opportunity to update the affected tests to our current style mode instead of classic mode. Let's see what others say, though.

labelspacing is relevant. Try to change it to a big number and see what happens.

I don't think you need the new kwarg or rcParam, and I think adding a new one is more confusing than adding yet another kwarg controlling the vertical spacing of legend elements. i.e. by default you set handleheight and labelspacing to None. If they are still None when they are set (l 408) then you set an internal variable _scalehandlebox to True, and set handleheight and labelspacing to their default values. If either of them is set then, _scalehandlebox is False and you just do what you did before.

I actually think its unfortunate that there is both a handleheight and labelspacing, since they both kind of do the same thing.

and set handleheight and labelspacing to their default values.

What is the default value if they are both None???? Do you want to hardcode some value into the code?

labelspacing is not relevant here in the sense that the PR does not touch it at all. Although maybe it should?

Yes if handleheight. is none, then the automatic algorithm is used and whatever value is needed for handleheight in the code is hard coded. I don’t see why it’s needed at all if we automatically determine the height of the handle but it certainly need not be a variable or if it does you will have to walk me through it

Thanks for explaining the technical diff between label spacing and handle height. What an unfortunate pair of names. Hence why I’m being a bit of a pita about the API here. I agree that this variable need not be part of this PR. But I still do not think that he height and width of the handles should have three kwargs controlling their size.

if handleheight is none, then the automatic algorithm is used and whatever value is needed for handleheight in the code is hard coded.

I would agree to that if "the algorithm" was a true optimization in both directions. However, as it is now it's only an expansion in one direction in case the marker is larger than the handleheight. Therefore there needs to be a "default" value which is used for all cases which are not using huge markers (which are like 99% of all cases I suppose). Hardcoding this default value would be a step back compared to where we are now, letting the rcParams decide the default.

The "algorithm" does expand both width and height.

Sorry, by "direction" I mean of course that it only expands - it does not shrink.

Hardcoding this default value would be a step back compared to where we are now, letting the rcParams decide the default.

Adding yet another confusing kwarg to legend would be a step backwards compared to having the code do the right thing via a simple API.

Right now a legend entry needs a certain height and it needs a certain spacing between entries. I'm fine with the spacing being left along.

Ideally, the "automatic" height should be the larger of the text height and the marker height. If the user wants to overide this with a smaller or larger height, they should be welcome to, using non-None values of the handleheight kwarg/rcParam. I don't think there is a separate need for a default height that the user needs to control. You seem to want to use this as a min height, below which the height won't drop, even if it will automatically expand; i.e. make it 0.5 unless it needs to be bigger. I don't think there is enough reason for that extra flexibility to deserve a new kwarg for a method that already has 26 optional kwargs.

Using the font size as the default from which to potentially enlarge the handleheight is an option.

What will determine the "standard" handlelength? The handle itself? This might work for markers, but not for lines, which usually are just as long as the handlelength. So if handlelength=None, how long will the legend for the usual plot be?
I would then also fear that one ends up with significantly different legends with markers compared to legends with lines or else one needs to hardcode a value, which is again undesired.

Again, I don't see a fundamental problem with an autoscale in the horizontal, unless the user specifies a handlelength either via rcParam or kwarg. Yes, that will mean that lines will have a hardcoded default length if you want the width to otherwise be adjustable. I still think thats an improvement on the present situation, and still gives the user a way to recover current behaviour.

Thanks a lot for the contribution. I'm going to close this as having had no action for quite a while. OTOH, if there is still a community that wants this, please feel free to re-open, with my apologies!