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

extend interval to dot and text marks #627

Merged
merged 4 commits into from
May 29, 2022
Merged

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Dec 21, 2021

closes #611

(not related to #597 which requires a very different kind of interval)

@Fil Fil requested a review from mbostock December 21, 2021 18:04
@Fil Fil mentioned this pull request Jan 1, 2022
src/transforms/interval.js Outdated Show resolved Hide resolved
@mbostock
Copy link
Member

mbostock commented Jan 1, 2022

Thanks for incorporating my suggested changes.

I’d love to have a better motivating example of how this should be used. In the case of plotting athletes by birthday month, I think I would probably recommend this instead:

Screen Shot 2022-01-01 at 10 21 33 AM

Plot.plot({
  marginLeft: 60,
  y: {
    reverse: true
  },
  marks: [
    Plot.rectX(athletes, Plot.binY({x: "count"}, {y: d => d.date_of_birth, thresholds: d3.utcMonth})),
    Plot.text(athletes, Plot.binY({x: "count", text: "count"}, {y: d => d.date_of_birth, thresholds: d3.utcMonth, dx: 4, textAnchor: "start"})),
    Plot.ruleX([0])
  ]
})

Alternatively, if I wanted an ordinal y-scale using the group transform:

Screen Shot 2022-01-01 at 10 21 54 AM

Plot.plot({
  y: {
    tickFormat: Plot.formatMonth()
  },
  marks: [
    Plot.barX(athletes, Plot.groupY({x: "count"}, {y: d => d.date_of_birth.getUTCMonth()})),
    Plot.text(athletes, Plot.groupY({x: "count", text: "count"}, {y: d => d.date_of_birth.getUTCMonth(), dx: 4, textAnchor: "start"})),
    Plot.ruleX([0])
  ]
})

I can reduce your example to this, which produces identical output to the first:

Plot.plot({
  marginLeft: 60,
  y: {
    reverse: true
  },
  marks: [
    Plot.rectX(athletes, Plot.groupY({x: "count"}, {y: d => d3.utcMonth.floor(d.date_of_birth), interval: d3.utcMonth})),
    Plot.textX(athletes, Plot.groupY({x: "count", text: "count"}, {y: d => d3.utcMonth.floor(d.date_of_birth), interval: d3.utcMonth, dx: 4, textAnchor: "start"}))
  ]
})

But, it still seems more complicated since you need to apply the month interval twice, whereas with the bin transform, you only need to specify it once, and the bin transform already computes the midpoint. Though… I guess that alone is a reason for the interval transform to do the same, for symmetry.

@mbostock
Copy link
Member

mbostock commented Jan 1, 2022

Also, and apologies for not realizing this sooner, we should find a way to reuse the existing mid helper (used by the bin transform) for the interval transform so that the two behave consistently:

plot/src/mark.js

Lines 277 to 292 in 72b2adb

// Assuming that both x1 and x2 and lazy channels (per above), this derives a
// new a channel that’s the average of the two, and which inherits the channel
// label (if any). Both input channels are assumed to be quantitative. If either
// channel is temporal, the returned channel is also temporal.
export function mid(x1, x2) {
return {
transform(data) {
const X1 = x1.transform(data);
const X2 = x2.transform(data);
return isTemporal(X1) || isTemporal(X2)
? Array.from(X1, (_, i) => new Date((+X1[i] + +X2[i]) / 2))
: Float64Array.from(X1, (_, i) => (+X1[i] + +X2[i]) / 2);
},
label: x1.label
};
}

@mbostock
Copy link
Member

@mbostock mbostock force-pushed the fil/interval-transform branch from 8462f14 to b1b2d67 Compare May 29, 2022 03:29
@mbostock mbostock changed the title extend interval to dot, text, image marks extend interval to dot and text marks May 29, 2022
@mbostock mbostock merged commit 9c6197e into main May 29, 2022
@mbostock mbostock deleted the fil/interval-transform branch May 29, 2022 04:27
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