Skip to content
This repository was archived by the owner on Jul 13, 2021. It is now read-only.

Conversation

@grahamas
Copy link
Contributor

@grahamas grahamas commented Feb 19, 2021

A GroupedBarPlot gbp takes x and groups and plots [barplot!(gbp, x .+ offset(group)), group; gbp.attributes..., color=color(group), width=width(group)) for group in groups], calculating the offset, the width, and the color such that the resulting plot has at each value of x a series of adjacent bars, one for each group in groups, colored by group identity.

Since at its core it just calls barplot!, all the BarPlot attributes should work as usual, except color and width, which are both overridden to specify a color or width for each group (though a scalar width does fix all the bar widths). Also I overrode Legend to create an appropriate legend given a list of group labels, but I haven't overridden label to take labels at time of plot creation.

TODOs:

@greimel
Copy link
Contributor

greimel commented Feb 20, 2021

Sorry that my other PR has stalled a bit. I guess this duplication of effort would have been avoidable.

Could you post a screen shot of a plot?

Do you implement stacking? (this is done in #580)
Do you implement incremental plotting? I.e., can you plot group by group in a loop? (this is missing from #580 because I couldn't think of a way to make this work with stacking)

@grahamas
Copy link
Contributor Author

groupedbarplot_cleaner

Ah I didn't do stacking. Part of my goal was to call barplot at the end of the day to use as much of its existing attributes handling as possible. It seems like stacking will require calling poly! directly, which it looks like is the approach you took. Is there a reason to do both at in the same plot type? They seem different enough to me that I'd want to implement GroupedBarPlot and StackedBarPlot.

I didn't do incremental plotting, either. From reading through yours, it looks like the point is to use AlgebraOfGraphics? I've never used it, but I'd be happy to add that in. It would require redrawing the whole plot, though. I'd be inclined to make that a second PR, though, since AFAIK that's not generally supported.

So questions to answer:

  • Do we need iterative plotting in the initial PR?
  • Do stacked and grouped (dodged?) barplots belong in the same function?
  • Does either belong in barplot, as in your PR? (@jkrumbiegel suggested no)
  • Are there any fundamental functionalities of barplot that one would expect in groupedbarplot that I'm overlooking?

The latter is what I'm inclined to think is the minimal question before merging.

@greimel
Copy link
Contributor

greimel commented Feb 21, 2021

Why shouldn't stacking and dodging just be keywords in barplot? Is there any downside? (@jkrumbiegel)

If we go with a separate recipe, then grouped bar plots should at least cover both stacking and dodging. That is what StatsPlots.jl and ggplot do.

The added benefit of your PR with respect to mine is the automatic legend. So what I'd propose is the following.

  • implement stack and dodge as keywords in barplot (as in my PR Add stacking and dodging to bar plot #580)
  • implement automatic legend on top of that (as in this PR, but checking if dodge, stack or color keyword should be used)

Of course it's up to @SimonDanisch and @jkrumbiegel to decide.

(Simon, I've removed the depency on DataFrames in the other PR so it's ready from my side.)

@jkrumbiegel
Copy link
Member

I don't know, my feeling is that barplot by itself should be simple. But maybe the usability of the all-in-one approach is greater. One factor could be that a GroupedBar recipe might need different conversions than just BarPlot. And that would be annoying or even impossible if the behavior depended on keyword settings, especially because you can change them afterwards.

@ffreyer
Copy link
Collaborator

ffreyer commented Feb 21, 2021

Why shouldn't stacking and dodging just be keywords in barplot? Is there any downside?

I think the arguments against it would be:

  • It's more difficult to use barplot (extra arguments necessary)
  • Something that worked before doesn't work anymore
  • barplot takes (much) longer (for the people that want to make tons of plots quickly)

If none of these are true it's probably fine to add that functionality directly.

@greimel
Copy link
Contributor

greimel commented Feb 21, 2021

I think the arguments against it would be:

  • It's more difficult to use barplot (extra arguments necessary)
  • Something that worked before doesn't work anymore
  • barplot takes (much) longer (for the people that want to make tons of plots quickly)

None of them is true. stack and dodge default to automatic in which case no additional work is done. So the addition should neither break old code, nor make it slower.

One factor could be that a GroupedBar recipe might need different conversions than just BarPlot. And that would be annoying or even impossible if the behavior depended on keyword settings, especially because you can change them afterwards.

Sorry, I don't understand what this means. Would you mind elaborating a bit?

@jkrumbiegel
Copy link
Member

I meant that you might want to convert different inputs differently depending on whether you are constructing a grouped or normal barplot. And the conversion can't depend on a keyword attribute

@greimel
Copy link
Contributor

greimel commented Feb 21, 2021

I see. What do you think, how frequently would that come up?

If it's very rare then the work around would be to build a GroupedBar recipe for this particular use case. This will be much simpler than this PR once #580 has landed, right?

Also, how is that different from any other keyword in any other recipe?

@jkrumbiegel
Copy link
Member

I've thought a bit more about it, maybe it's not so problematic with the conversions as the critical types would be sent to grouping / stacking attributes anyway. The positional argument types should be the same I guess

@jkrumbiegel
Copy link
Member

Also, how is that different from any other keyword in any other recipe?

I think we don't have keyword-dependent conversion in any recipe

@grahamas
Copy link
Contributor Author

grahamas commented Feb 21, 2021

It will definitely be a different conversion: the natural signature is barplot(::Vector{T}, ::Vector{::Vector{T}}; type=:grouped) and that doesn't work with the existing point-based conversion method. It's not toooo problematic, maybe, because in theory you could just define convert_arguments(pb::PointBased, x::AbstractVector{T}, ys::AbstractVector{<:AbstractVector{T}}) = [convert_arguments(pb, x, y) for y in ys] though it may be a little... weird.

edit: You could override convert_arguments(::BarPlot, ::AbstractVector{T}, ::AbstractVector{<:AbstractVector{T}}), which would probably be cleaner if still weird.

@jkrumbiegel
Copy link
Member

In your version, yes, in @greimel's version #580 I think the signatures are still the same. I guess we should converge on one of these sooner rather than later so that nobody wastes time. Currently, I think I'd tend towards the version from @greimel because I suspect now that extending barplot might just be the simplest solution.

@grahamas
Copy link
Contributor Author

Ohhh I see-- when you're coming from DataFrames, it makes sense to have x and y and group_id all same sized vectors.

I don't have any real feelings either way about my or @greimel 's version being the more appropriate implementation (though having stacking and grouping at the same time seems nice) but I do feel very strongly that you shouldn't need a DataFrame to plot, so I would strongly advocate for a wrapper around griemel's implementation to allow the signature I use. It's just the natural approach when you're not using a DataFrame (read: I don't want to construct a group identity vector nor duplicate my x vector). I'd be happy to implement that part once griemel's implementation is merged.

@greimel
Copy link
Contributor

greimel commented Feb 22, 2021

would you mind providing the code snippet that generates a plot (including dummy data) your way so that I better understand your approach?

@grahamas
Copy link
Contributor Author

grahamas commented Feb 22, 2021

Sure! The above plot is produced by:

gbp = groupedbarplot(1:4, [2:5, 3:6, -1:2])
gbp.axis.xticks = 1:4
axislegend(gbp.axis, gbp.plot, ["decent", "best", "worst"])

edit: Though don't try to clone and replicate. The code works when external to AbstractPlotting, but not inside of it

@greimel
Copy link
Contributor

greimel commented Feb 24, 2021

In the long-run, this will be supported by AlgebraOfGraphics (using the slicing context)

visual(BarPlot) * mapping(1:4, [2:5 3:6 -1:2], dodge = dims(2))

This needs #580 to be merged and AlgebraOfGraphics get a non-incremental mode (which might come through https://github.com/greimel/TabularMakie.jl)

Implementing this directly would probably be against the Makie philosophy. It can neither plot lines nor grouped scatter plots that way. So there should probably rather be a general solution for grouping.

@piever
Copy link
Member

piever commented Feb 26, 2021

An additional design consideration is that these features should not be limited to bar plots. Dodging also makes sense for boxplots and violin plots, and stacking also makes sense for the histogram recipe (but maybe that part we get for free with this change), so we should make sure that we can implement all of those without too much code duplication.

@jkrumbiegel
Copy link
Member

as @greimel's version is now merged, I'll close this PR. Thanks for your effort, that certainly helped getting to a good design which I hope has now been found

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants