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

ordinal interval #849

Merged
merged 7 commits into from
Jun 10, 2022
Merged

ordinal interval #849

merged 7 commits into from
Jun 10, 2022

Conversation

mbostock
Copy link
Member

Fixes #513.

Screen Shot 2022-04-18 at 5 06 29 PM

When a scale has an interval option, interval.floor is used as the scale transform, and then for ordinal scales, the domain is computed using interval.range(min, max), ensuring that gaps are shown and that ordinal values appear in the intended order.

@mbostock mbostock requested a review from Fil April 19, 2022 00:07
@Fil
Copy link
Contributor

Fil commented Apr 19, 2022

Taking a bit more time than expected because it has implications on the warnings; for example if you have a date interval on x you don't want a warning that the dates are unexpected in an ordinal scale.

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

This is going to be so useful! Solving a good part of the trouble with ordinal years.

#852 complements this PR by suppressing the warning on ordinal dates in the case of a scale interval.

@Fil
Copy link
Contributor

Fil commented Apr 20, 2022

A few more suggestions, maybe for a fast follow:

  • Should we bail out if this generates tens or hundreds of thousands of ticks? Maybe you passed dates and say "interval: 1" because in your mind it's one day, and suddenly plot wants to compute millions of milliseconds? (If this reaches billions, the browser might bail out by itself, throwing RangeError: Invalid array length, but there is a range where it will try to compute hundreds of thousands of dates.) I think it would be fair to cap this at 1000 or even 5000.

  • If x has an interval: 1 (but still a "continuous" mark), then it should be an indication that the user doesn't want fractional ticks? This would feel like a natural way of solving When all the values are integers, skip fractional ticks? #355; likewise with interval: 10 we would only want ticks that are multiples of 10; similar story for date intervals.

  • The scale interval creates a transform that is applied after stacking, resulting in non-stacking bars. Which makes me want to allow stacking of events that would belong to the same bar although they don't have the exact same x. Note that using "interval" inside a Plot.barY's options is meant for the other axis. I don't know how we can address this issue.

@mbostock
Copy link
Member Author

Should we bail out if this generates tens or hundreds of thousands of ticks?

Should this be a more general error if you have a Very High Cardinality Domain (VHCD) on an ordinal scale? Or maybe just for a point or band scale? I think it’d be pretty easy to add this check in the scale constructor when the domain is not set explicitly.

If x has an interval: 1 (but still a "continuous" mark), then it should be an indication that the user doesn't want fractional ticks?

If x has interval: 1, then it will have a default transform of Math.floor, so you shouldn’t get any fractional values in the domain (unless you set the domain explicitly).

The scale interval creates a transform that is applied after stacking, resulting in non-stacking bars.

Yeah, I think this is a general confusion around scale transforms, which is that they happen after any mark data transforms. It might be better if they happened before instead, but I don’t think that’s possible (since the scale options aren’t exposed to mark data transforms). I think maybe we just live with this one.

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

The interval option should be exposed with plot.scale("x"); it is not enough to expose the materialized domain: we are missing the derived transform option.
(A way to solve this is suggested in #852, with tests)

Fil and others added 3 commits June 3, 2022 17:33
* Specifying a scale interval shows the intent of having ordinal numerical or ordinal dates: suppress warning.

Side note: if a numeric interval was specified, string numerics have already been coerced to numbers by the scale transform; so this in fact only has consequences for ordinal dates, such as in the downloads-ordinal test plot.

* document scale intervals

* test plot with year intervals

* Update src/scales.js

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

* Update src/scales.js

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

* Update src/scales.js

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

* d3.utcDay-like intervals do not parse string dates

* reusable interval option

* When the interval option is applied on a quantitative scale, generate the ticks with the interval; also set the tickFormat so that we don't show 1.0, 2.0, 3.0 if the interval is an integer.

* tests

* normalize intervals

lists a few TODOs re: the default tick format:
- we don't want decimal notation if the interval is specified as an integer
- we don't want months to appear if the interval is specified as d3.utcYear
- we don't want years to appear with commas (#768)

* formatDefault for ordinal scales

* Update README

* call maybeInterval sooner

* tabular-nums for interval’d ordinal axes

Co-authored-by: Mike Bostock <[email protected]>
@mbostock mbostock force-pushed the mbostock/ordinal-interval branch from 48166af to 51741f4 Compare June 10, 2022 15:39
@mbostock mbostock requested a review from Fil June 10, 2022 15:42
@mbostock
Copy link
Member Author

This should be ready to go.

@Fil
Copy link
Contributor

Fil commented Jun 10, 2022

Excellent! The only thing I don't like is the formatting of integers as 1.0, 2.0 etc in the test/output/integerInterval.svg plot. I hoped we could use the interval to infer that the format should not include the decimal dot—but that will be for another issue.

@mbostock mbostock merged commit 4735253 into main Jun 10, 2022
@mbostock mbostock deleted the mbostock/ordinal-interval branch June 10, 2022 22:00
mbostock added a commit that referenced this pull request Jun 24, 2022
* bail out if the inferred ordinal domain has more than 10k values
(see #849 (comment))

* only on point and band scales

* only throw an error for position scales

* apply review suggestion: key as first argument to ScaleO

* shorter error message

Co-authored-by: Mike Bostock <[email protected]>
This was referenced Apr 27, 2023
frontend-provider pushed a commit to frontend-provider/plot that referenced this pull request Sep 20, 2023
* bail out if the inferred ordinal domain has more than 10k values
(see observablehq/plot#849 (comment))

* only on point and band scales

* only throw an error for position scales

* apply review suggestion: key as first argument to ScaleO

* shorter error message

Co-authored-by: Mike Bostock <[email protected]>
backend-devloper pushed a commit to backend-devloper/plot that referenced this pull request Nov 24, 2023
* bail out if the inferred ordinal domain has more than 10k values
(see observablehq/plot#849 (comment))

* only on point and band scales

* only throw an error for position scales

* apply review suggestion: key as first argument to ScaleO

* shorter error message

Co-authored-by: Mike Bostock <[email protected]>
tigrevol8888 added a commit to tigrevol8888/plot that referenced this pull request Jul 5, 2024
* bail out if the inferred ordinal domain has more than 10k values
(see observablehq/plot#849 (comment))

* only on point and band scales

* only throw an error for position scales

* apply review suggestion: key as first argument to ScaleO

* shorter error message

Co-authored-by: Mike Bostock <[email protected]>
LordOfCodes26 added a commit to LordOfCodes26/plot that referenced this pull request Aug 15, 2024
* bail out if the inferred ordinal domain has more than 10k values
(see observablehq/plot#849 (comment))

* only on point and band scales

* only throw an error for position scales

* apply review suggestion: key as first argument to ScaleO

* shorter error message

Co-authored-by: Mike Bostock <[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.

Ordinal intervals?
2 participants