Thanks! I think this idea has been discussed before. I'm not sure of the technical issues, but in general it seems reasonable.

Some of your image changes are quite large according to the tests so it would be good to see before and after and decide what is wrong. The way to do that is to check in the new images as part of the PR (in tests/baseline_images) and we can see the difference.

FWIW this will also break all of the seaborn tests that assert the correctness of bar/hist plots by checking the attributes of the patch artists.

I don't know if there is some way to keep a "dummy" reference to artists in their original place that does not trigger a duplicate draw ... could be useful?

@jklymak thanks for the quick look! I've added a TODO to the top of the PR with the problems I've noticed in the test plots.

Currently looking at legends forhist. Not totally sure how to approach it, but am currently thinking of modifying this part ofAxes.hist

# If None, make all labels None (via zip_longest below); otherwise,# cast each element to str, but keep a single str as it.labels= []iflabelisNoneelsenp.atleast_1d(np.asarray(label,str))forpatch,lblinitertools.zip_longest(patches,labels):ifpatch:p=patch[0]p.update(kwargs)iflblisnotNone:p.set_label(lbl)

And instead adding a new something (Artist I guess?) to the plot from the patches that would have been assigned labels.

labels= []iflabelisNoneelsenp.atleast_1d(np.asarray(label,str))legend_patches= []forpatch,lblinitertools.zip_longest(patches,labels):ifpatch:p=patch[0]p.update(kwargs)iflblisnotNone:p.set_label(lbl)legend_patches.append(p)forpinpatch[1:]:p.update(kwargs)p.set_label('_nolegend_')self.add_somehow(legend_patches)

Is there a more elegant way to do this?

@mwaskom, is it possible to expand the collections into lists of patches? Maybe a helper around this?

Changing the return type is definitively an API change. Any user who is trying to do something with the returned is going to have their code broken.

You might be able to sub-class the Collection and make it look enough like the current return type, but it will be a lot of work as things like

bars = ax.bar(....)bars[1].set_color('green')bars[2].set_visible(False)bars[3].set_linewidth(15)*_, last, two = bars

all should keep working.

If your problem is primarily histograms@ivirshup you may do better either passinghisttype='step' or using the new in mpl34ax.stairs method. If you really want the bar look on histograms adding a new histtype (and accepting a bit more opt-in type instability inhist) may be easier to get done than changing the return type of allbar calls.

maybe we could haveax.bar(..., quick=True)? Thoughax.histbar isn't a bad idea.

@tacaswell, my thing was mainly that I didn't want to have to consider the size of my data and choose the keyword arguments. Particularly as a library author, having changes to the plots around arbitrary data size cutoffs is not going to be fun to maintain. Also I think people generally are expecting something that looks like bars when the bins are wide enough to be able to tell.

I think your point is totally fair. I wasn't really sure what was considered to be key parts of the API though, since arguably any changes to public attributes of the object is breaking. I wasn't planning on changing the return type, just it arguably wouldn't be a particularly useful value. Basically, my thinking was if changing the contents ofax.containers isn't breaking, is changing the behaviour ofres = ax.bar(...); res[0].set_facecolor(...) breaking? But also, if the first is breaking, it would be difficult to make any improvements to this library.

TBH, if this got rejected here, my plan was to see if@mwaskom would be up for replacing the use ofax.bar inseaborn with someCollections, since initial tests make it look like this will cut plot time for large histograms by at least 50%.

From looking at the code a bit, I was actually wondering if the plan was to deprecate thecontainers module, since it's only used in a small subset of the library (specifically for bar and stem charts, I think). A little confusingly, some things that are annotated asContainer actually expect subclasses ofToolContainerBase which is unrelated.

My point here being, maybe this would be worth it for 4.0? It can be quite slow at the moment, and the machinery for making it fast is all here. Using a Collection instead of a Container would also bring this function more in line with the rest of package.

Every change that can break or change the result of currently valid user code is a breaking change to us.

if the first is breaking, it would be difficult to make any improvements to this library.

Unfortunately, it is. This is the downside of a very large user base. We have to be extermely careful with these.

Using a Collection for the bars is reasonable. Probably the way forward is similar tohist(..., histtype=...): Add a parameter to make the Artist configurable. The default stays on isolated bars, using a collection is opt-in. The default can be switched later.

A clever in between way may be returning a proxy incontainer.patch, that will resolve the PatchCollection into individual rectangles when trying to access individual elements. If the user does not mess with individual rectangles, he'll get the speedup, otherwise he'll get the current behavior. This is also not trivial to get right, but seems more managable than trying to write a Rectangle adapter mapping to the collection.

@ivirshup are you still interested in working on this (under the constraints of backward compatibility and the paths laid out in my last comment)?

@timhoffm, I don't think so. Many of the solutions suggested above got fairly involved, and I don't think I'm familiar enough with matplotlib internals to do this in a reasonable timeframe.

Feel free to close, though I think the conversation here definitely has value.

@ivirshup thanks for the feedback. I've created issue#22976 with a concise description of the suggested solution. Therefore, I close this thread.