plotly.js icon indicating copy to clipboard operation
plotly.js copied to clipboard

make offsetgroup work with barmode 'stacked' and 'relative' for bar traces

Open my-tien opened this issue 1 year ago • 11 comments
trafficstars

This change makes it possible to create groups of stacked bar traces by setting barmode: "stacked" (or relative) on the axis and setting the same bar.offsetgroup for bars that should stack together. If no offsetgroup is defined, all bars at the same position are stacked together.

To summarize the interaction between axis.barmode and bar.offsetgroup after this change:

barmode 'group': bars without offsetgroup are automatically offsetted to appear next to each other. If some bars should appear at the same location, they should have the same offsetgroup.

other barmodes: bars without offsetgroup share the same position on the axis. If some bars should appear next to each other, they should have differing offsetgroups.

my-tien avatar Jun 01 '24 14:06 my-tien

@my-tien the mapbox baseline failures are fixed in #7019. Please fetch upstream/master and then merge master into this pull request as well as into other PRs.

archmoj avatar Jun 06 '24 17:06 archmoj

Getting closer. Could you please investigate why the legendgroup mock renders differently compared to master?

archmoj avatar Jun 06 '24 17:06 archmoj

Thanks @archmoj, I was able to restore the original looks of the legendgroup mock.

Two general questions to ensure that my changes make sense:

  1. I am a little bit unsure about the intended interaction between different trace types and their bar/scatter/box...modes. As far as I understand it, across traces we have the implicit behavior of "overlay" (with the exception of bars and histograms, because histograms also use barmode). Is this correct?

  2. How should alignmentgroup work exactly? The description in the documentation wasn't clear to me. And should alignmentgroups only matter among traces of the same type? I was experimenting with bars and boxplots in this codepen and saw some interaction. Boxplots become invisible when a bar shares its alignmentgroup: https://codepen.io/toffi-fee/pen/zYVaKrB

EDIT: I accidentally overwrote the content of the previously linked codepen with something else. Now I've recreated my example:

image

And here is the output of the same figure in my branch:

image

my-tien avatar Jun 07 '24 13:06 my-tien

@alexcjohnson Hi Alex, do you have input on my last question above? Currently I am not sure what the desired behavior is.

my-tien avatar Aug 26 '24 09:08 my-tien

Yes, somewhere we need more complete documentation of what alignmentgroup and offsetgroup really mean and do (cc @LiamConnors - also in writing this up I found a bug https://github.com/plotly/plotly.js/issues/7133). Let me take a shot at it here:

  • Traces of any type in the same alignmentgroup take each other into account when determining their width (for all but scatter) and position. Traces with different alignmentgroup will not influence each other's width or position in any way.
  • Within one alignmentgroup, the available width is divided up according to the number of distinct offsetgroup values found among the traces in that alignmentgroup.
    • All traces of any type (other than scatter) in the same alignmentgroup are given the same width.
    • All traces of any type in the same alignmentgroup AND offsetgroup are given the same position.
    • The available width is the minimum difference between any of the distinct position coordinates among all traces of any type in that alignmentgroup, reduced by the appropriate gap value.

I am a little bit unsure about the intended interaction between different trace types and their bar/scatter/box...modes. As far as I understand it, across traces we have the implicit behavior of "overlay" (with the exception of bars and histograms, because histograms also use barmode). Is this correct?

The default for scattermode, boxmode, and violinmode is overlay - and currently in that case all of this grouping is ignored, you need to explicitly set them to group for any of this to matter. I'm not sure that's the right behavior, particularly given your goal here of allowing stacking and grouping simultaneously for bars, but I think it's OK to leave as is for those three types since they only have two mode values (overlay and group) and in group mode you can recover overlay behavior for an individual trace by giving it a unique alignmentgroup. If you don't provide any offsetgroup values it makes the most sense to put one trace of each type into each implicit offsetgroup, rather than making a new offsetgroup for each trace regardless of type, because typically different trace types are used to show different information about the same underlying objects. I would also say that no trace with a missing offsetgroup or alignmentgroup should be implicitly given the same value as an explicitly-provided offsetgroup or alignmentgroup.

How should alignmentgroup work exactly? The description in the documentation wasn't clear to me. And should alignmentgroups only matter among traces of the same type? And should alignmentgroups only matter among traces of the same type? I was experimenting with bars and boxplots in this codepen and saw some interaction. Boxplots become invisible when a bar shares its alignmentgroup: https://codepen.io/toffi-fee/pen/zYVaKrB

Both of those look wrong to me based on the rules above:

Clearly something is wrong if the boxes in the same alignmentgroup as the bar disappear. But it's also wrong if the bar doesn't have a consistent width with the boxes, and if no offsetgroups are provided, the bar trace should be aligned with the first box trace. If I take your codepen, put everything into the same alignmentgroup, and provide all traces with explicit offsetgroups, the bar matching the first box trace, I get what I consider correct behavior (with the exception that the bar and box widths and alignment are a little off - this is bug #7133 again - because the default boxgap is 0.3 whereas the default bargap is 0.2). https://codepen.io/alexcjohnson/pen/xxozBJb?editors=0010 Screenshot 2024-08-27 at 11 22 58

Explicitly setting boxgap=0.2 it looks exactly correct: Screenshot 2024-08-27 at 11 23 33

Also, clearly something is wrong if boxes with different alignmentgroup influence each other's position and width - as is the case in your codepen since the third box trace is in alignmentgroup="2" whereas the other two are in alignmentgroup="1". Again, providing explicit offsetgroups makes this look correct to me, in that the last box trace takes up the whole width because it's the only one in its alignmentgroup, and the other three split the width evenly because they're all in the same alignmentgroup with different offsetgroup (again, except for #7133) - so I think the root bug here is just how we deal with missing offsetgroups. https://codepen.io/alexcjohnson/pen/MWMXxxR?editors=0010 Screenshot 2024-08-27 at 11 31 29 This last one is what I think your codepen, exactly as it's written right now, should look like if we fixed this bug.

alexcjohnson avatar Aug 27 '24 15:08 alexcjohnson

Thanks @alexcjohnson for your comprehensive analysis!

2 remarks:

If you don't provide any offsetgroup values it makes the most sense to put one trace of each type into each implicit offsetgroup, rather than making a new offsetgroup for each trace regardless of type,

To make sure I understand you correctly: Given a chart with bars A, B, C and scatters X, Y, Z, the following traces should share an implicit offsetgroup, correct?

  • A and X
  • B and Y
  • C and Z

  1. Below, I think you meant to say

Also, clearly something is wrong if boxes with ~~the same~~ a different alignmentgroup influence each other's position and width


I think the behavior you describe sounds reasonable and will try to incorporate it in my PR. For bars I will implement the behavior as follows:

barmode group: bars without offsetgroup receive a different implicit offsetgroup each. Bars with same offsetgroup overlay. other barmodes: bars without offsetgroup share the same implicit offsetgroup. Bars with same offsetgroup stack or overlay depending on the mode.

my-tien avatar Aug 28 '24 09:08 my-tien

@my-tien yes, that’s all correct, and thank you for catching my error, I will fix it above.

alexcjohnson avatar Aug 28 '24 12:08 alexcjohnson

After spending way too much time trying to implement the implicit alignment and offsetgroups I figured that I don't actually need that for this PR. As long as alignmentgroup and offsetgroup are set explicitly for all traces, they work as expected.

Regarding the two still failing baseline tests, the new images look to me more like what I would expect based on their mocks (ticks and grid at whole numbers). What do you think?

If you agree, I'll replace the baseline images with the new ones.

E.g. compare current baseline:

image

New image:

image

my-tien avatar Sep 02 '24 10:09 my-tien

@my-tien Would you please fetch upstream/master and merge it into this branch?

archmoj avatar Sep 09 '24 19:09 archmoj

done @archmoj

my-tien avatar Sep 10 '24 07:09 my-tien

@my-tien Would you please fetch upstream/master and merge it into this branch again? We are thinking to possibly include this feature in v3.1.0. Thank you!

archmoj avatar Oct 18 '24 14:10 archmoj

Thinking about this PR, it looks like a change that could possibly include in v3.0.0. @gvwilson @my-tien Would you please merge master into this branch so that the tests pass? Also please add draft log with a _change suffix. Thank you!

archmoj avatar Oct 20 '24 16:10 archmoj

@archmoj: I opened https://github.com/plotly/plotly.js/pull/7247 and rebased the branch on the latest master (I don't have access to my-tien's original branch). If you want, you can merge the new PR https://github.com/plotly/plotly.js/pull/7247 and close this one. I also added a draft log with _change suffix as requested.

stephprobst avatar Oct 22 '24 14:10 stephprobst

@archmoj: I opened #7247 and rebased the branch on the latest master (I don't have access to my-tien's original branch). If you want, you can merge the new PR #7247 and close this one. I also added a draft log with _change suffix as requested.

Thanks @stephprobst I'm working on the same thing right now.

archmoj avatar Oct 22 '24 14:10 archmoj

:+1: on including this in 3.0 cc @LiamConnors we'll have to document the changed behavior

gvwilson avatar Oct 22 '24 17:10 gvwilson

Many thanks for wrapping this up in my absence! <3

my-tien avatar Nov 04 '24 09:11 my-tien