Skip to content
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

difference as a composite mark #1897

Merged
merged 3 commits into from
Oct 24, 2023
Merged

difference as a composite mark #1897

merged 3 commits into from
Oct 24, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Oct 19, 2023

this allows to use all the smarts in areaY and lineY (e.g. default tip orientation, variable fill color).
Also adds:

  • a test for the curve+tension.
  • channels for the default tip, reflecting top value, bottom value and difference.

(I'd still want to remove the custom area generator but I'm not sure how.)

TODO:

  • make sure to only pass the relevant options to the area (for instance, we don't want to pass stroke through); maybe only curve+tension?
  • use a proper render transform (for composability)

Note that a difference in the snapshot tests is a 0.5px realignment of the main line—which I think is correct in this branch.

@Fil Fil requested a review from mbostock October 19, 2023 10:04
@mbostock
Copy link
Member

This technique computes the channels multiple times, which isn’t quite right (e.g., if you have a nondeterministic channel specification that uses Math.random). I’ll think some more but I think we want to stick with a single mark while improving the tip behavior and supporting variable fill/stroke. I didn’t consider the latter a priority and hence punted on it, but I can work on it if you think it’s a blocker.

@Fil
Copy link
Contributor Author

Fil commented Oct 21, 2023

I wanted to explore a composite mark, because this looked like the perfect candidate for one, an solved "for free" the pending questions (variable fill) and the small alignment issue.

These can certainly be addressed differently, but composability is a great feature of Plot and if we can't use it in this case, it's a good indication that we need to make it work better. It would be great if a composite mark could share sub-mark channels, and it would also benefit the box mark.

I don't think this is blocking, it was just my way of reviewing your PR and going off on a tangent :)

@Fil Fil mentioned this pull request Oct 21, 2023
9 tasks
@mbostock
Copy link
Member

I agree we often have this wrinkle with composite marks. And we do have the context.getMarkState method, so it should already be possible to share the channel definitions across marks using an initializer. It would also be possible to implement it as a channel transform, I suspect. Perhaps that is worth investigating.

@Fil
Copy link
Contributor Author

Fil commented Oct 23, 2023

I'm now using context.getMarkState to compute the channels once.

function renderArea(X, Y, y0, {curve}) {
return shapeArea()
.curve(curve)
.defined((i) => i >= 0) // TODO: ??
Copy link
Member

Choose a reason for hiding this comment

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

The -1 is populated by the groupIndex helper, which is used by the area and line marks:

plot/src/style.js

Lines 265 to 271 in 7098bf3

// If any channel has an undefined value for this index, skip it.
for (const c of C) {
if (!defined(c[i])) {
if (Gg) Gg.push(-1);
continue out;
}
}

Suggested change
.defined((i) => i >= 0) // TODO: ??
.defined((i) => i >= 0)

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.

The two techniques in maybeDifferenceChannelsY (using Plot.column and capturing the materialized columns in a transform) and the render transforms (using context.getMarkState) are different. I expect we could use the Plot.column technique in both places, which I expect would be more understandable/less powerful than the render transform technique. It’s usually more flexible to populate the channel values as soon as we can, in case the user wants to apply their own transforms. We could maybe even do it using a channel transform like maybeIntervalK does.

@mbostock mbostock merged commit c6c0be5 into mbostock/difference Oct 24, 2023
@mbostock mbostock deleted the fil/difference branch October 24, 2023 00:13
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.

2 participants