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

interval-aware transforms #1511

Merged
merged 6 commits into from
May 1, 2023
Merged

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Apr 30, 2023

In this approach, transforms are passed the plot options as an additional argument; this allows the group transforms (and likely other transforms) to check if the scale has a declared interval option, and if so, apply that interval before grouping. This is safe to do because applying an interval is idempotent.

image

Plot.plot({
  x: {tickFormat: "%Y", interval: "5 years"},
  marks: [Plot.barY(olympians, Plot.groupX({y: "count"}, {x: "date_of_birth"}))]
})

I’m leaving this as a draft because we need to think through what other transforms need this logic. Definitely the stack transform, at least… It’s ready.

This looks more promising than #1497. Previously #513 #849.

@mbostock mbostock requested a review from Fil April 30, 2023 02:46
return Plot.plot({
marginBottom: 65,
x: {
interval: "day",
transform: (d) => d3.utcDay.floor(d3.isoParse(d)),
Copy link
Member Author

Choose a reason for hiding this comment

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

This technique no longer works here because, again, the scale transform is applied after the mark transform, so if we want the mark transform to apply the scale’s interval, we need to parse the dates before the mark transform runs. Since this technique was never recommended (as documented), I think it’s acceptable to change the behavior, and I rewrote this example to use the recommended technique instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: we could make the previous version of ibm-trading work again if the "day" interval was allowed to read strings (as discussed in #1326).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but as we discussed previously, then interval: d3.utcDay would not behave the same as interval: "day", and I don’t think we should go down further the slippery slope of implicit coercion when it’s better to teach users to set their types earlier.

@mbostock mbostock marked this pull request as ready for review April 30, 2023 16:40
@mbostock
Copy link
Member Author

Okay, I added support for interval-aware grouping on x and y to the bin and stack transforms, and I think this is now ready for review. 👍

It’s possible in the future we might want to add interval-aware grouping for z when implicitly defined as fill or stroke and when the color scale has a declared interval. But I think that is rare, and it is reasonable for us to punt now to keep things simple.

@mbostock mbostock mentioned this pull request Apr 30, 2023
7 tasks
export async function intervalAwareGroup() {
const olympians = await d3.csv<any>("data/athletes.csv", d3.autoType);
return Plot.plot({
x: {tickFormat: "%Y", interval: "5 years"},
Copy link
Member Author

Choose a reason for hiding this comment

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

We can drop the tickFormat: "%Y" when #1512 is merged.

@mbostock mbostock force-pushed the mbostock/interval-aware-transforms branch 2 times, most recently from 7ce84f7 to fbce8aa Compare May 1, 2023 17:49
@mbostock mbostock force-pushed the mbostock/interval-aware-transforms branch from fbce8aa to b190419 Compare May 1, 2023 18:04
@Fil
Copy link
Contributor

Fil commented May 1, 2023

This seems to open the door to many more (custom) behaviors in the transforms. Which could be immensely powerful—I can now imagine a transform that would simplify a geo based on chart width, for example? Are we comfortable with transforms reading options.marks? Or (ab)using this to mutate options (e.g. writing options.width changes the chart's width)?

@mbostock
Copy link
Member Author

mbostock commented May 1, 2023

I definitely wouldn’t support mutating options, but reading options, yes.

@mbostock
Copy link
Member Author

mbostock commented May 1, 2023

Specifically regarding mutation, it’s discouraged because it would break sharing options across plots, and could cause nondeterministic behavior if the same options object were used multiple times. The behavior of mutation would also depend on the order of initialization of the marks. (And it’s highly surprising.)

What we would do instead is have the transform provide hints on channels, like what we do for changing which symbol set we use for filling vs. stroking. And then Plot can collect the hints across all channels for the entire plot and decide how to change the default values for missing options. That lets Plot decide holistically based on all the marks/hints, generating warnings if there are conflicts, etc.

@mbostock mbostock force-pushed the mbostock/interval-aware-transforms branch from 44b5ad0 to b190419 Compare May 1, 2023 22:34
* document interval aware transforms

* API doc

* Update scales.md

* restore caution

* describe interval-aware transforms

* types for custom transform functions

---------

Co-authored-by: Mike Bostock <[email protected]>
@mbostock mbostock merged commit 477cfd1 into main May 1, 2023
@mbostock mbostock deleted the mbostock/interval-aware-transforms branch May 1, 2023 23:55
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* interval-aware group transform

* interval-aware bin, stack

* rename to interval-aware

* fix key reversal

* remove redundant tickFormat

* document interval-aware group transform (observablehq#1518)

* document interval aware transforms

* API doc

* Update scales.md

* restore caution

* describe interval-aware transforms

* types for custom transform functions

---------

Co-authored-by: Mike Bostock <[email protected]>

---------

Co-authored-by: Philippe Rivière <[email protected]>
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