Skip to content

stacks #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 37 commits into from
Mar 3, 2021
Merged

stacks #177

merged 37 commits into from
Mar 3, 2021

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 1, 2021

(this is #132, rebased on #176)

  • Negative values always result in diverging stacking. (By contrast, in d3-shape an explicit diverging offset must be invoked.)
  • Offsets: expand and silhouette are similar to d3-shape, but taking into account negative values. wiggle…
  • Series can be grouped by z, fill, stroke, title…
  • The cumulative sum happens in a configurable ranking order; built-in orders are:
  • data order (will be passed up to the stackArea or stackLine mark)
  • stacking works as expected with faceting (see https://observablehq.com/@data-workflows/u-s-population-by-state-1790-1990)

changes wrt #132:

See API documentation at https://observablehq.com/@data-workflows/plot-stack-options

Test plots:

supersedes #110
supersedes #132
closes #109
closes #22
closes #113

@Fil Fil requested a review from mbostock March 1, 2021 13:00
@Fil Fil mentioned this pull request Mar 1, 2021
@Fil Fil added this to the Friends Preview milestone Mar 1, 2021
Base automatically changed from mbostock/nugroup to main March 1, 2021 20:22
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two outstanding questions:

There are a number of places that call array.indexOf (the sort option, rank = sum, rank = appearance, rank = insideOut, and rank = array of values). This probably means quadratic performance. Can we implement these so they are more efficient?

Do we need the sort option? There aren’t any examples that use it yet. That would eliminate one of the uses of array.indexOf, too.

@Fil
Copy link
Contributor Author

Fil commented Mar 2, 2021

The sort option is used only in the case of the wiggle offset, which tries to minimize the differences from one location to the next. We could ignore it, with a caveat that the wiggle offset needs data sorted on x to get the correct result. And in any case, the option is passed up to areaY.
Re: indexOf, most of those could be changed to a Map, where performance is a concern?

@mbostock
Copy link
Member

mbostock commented Mar 2, 2021

The sort option is a subset of the transform composition problem we were discussing in the other thread. I think it’s best to remove the option for now and instead implement a more general solution such as an array of transforms. Also, while the sort option is passed through to the next mark, it will be ignored because Plot.stack defines the transform option which takes priority over the sort option; the sort option is just shorthand for a transform, but is ignored if a transform is explicitly specified.

Yes, I think we should use a Map or similar instead of indexOf. Are you able to do that this morning, or should I?

@Fil

This comment has been minimized.

@mbostock
Copy link
Member

mbostock commented Mar 2, 2021

Okay, a few last (?) things:

  • The offset option should be coerced to a string and lower-cased (if not nully), similar to how we parse the curve option for line and area, and scheme for color scales.
  • The rank option similarly needs some sort of normalization, but it’s more complicated because we don’t want to lower-case if it’s a field name (and I’m still wondering if we can avoid ambiguity somehow).
  • The rank.indexOf test should be replaced… There isn’t a strong array-or-iterable test, so maybe we should just assume it’s an array of iterable if it’s not a string, null, undefined, or a function. We want it to crash if it’s not one of the expected values.

@mbostock
Copy link
Member

mbostock commented Mar 2, 2021

Oh, and z || fill || stroke || title probably needs to use undefined or null testing.

@mbostock
Copy link
Member

mbostock commented Mar 3, 2021

Almost there. Question: should rank be renamed to order to match D3’s stack.order? I know they are not exactly the same, but I don’t totally grok whether “rank” is a more expressive term than “order” here, or if it’s different just for the sake of being different. Related #179.

@mbostock
Copy link
Member

mbostock commented Mar 3, 2021

I realized I misunderstood the behavior of the order (formerly rank) option: when an array of values, it’s effectively an ordinal domain for the z dimension; it’s a partial ordering of series rather than a complete ordering of data as suggested in #179. This also means that the array interpretation is different than the function and field variants, as the latter produce complete orderings. I’m going to break things for a bit as I think through this design some more.

@mbostock mbostock merged commit 67f030f into main Mar 3, 2021
@mbostock mbostock deleted the fil/stack3 branch March 3, 2021 03:00
}
}
return positions(Z, order);
return positions(Z, Kn.reverse().concat(Kp));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naïve Q: I don't understand what this optimizes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array.unshift is an O(N) operation on an array of length N, so if it’s done O(N) times the total is O(N²). Where as array.push is O(1) and array.reverse and array.concat are O(N), so the total is O(N). It won’t matter much in practice since N is typically very low here (the number of distinct values in z), but it’s not hard to do efficiently either, so I did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faceted stacks Diverging stack? Stacked charts: area, bars, histogram?
2 participants