Skip to content

date scale interval & warning #852

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 15 commits into from
Jun 10, 2022
Merged

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Apr 19, 2022

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.

…cal 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.
@Fil Fil requested a review from mbostock April 19, 2022 15:08
@Fil
Copy link
Contributor Author

Fil commented Apr 19, 2022

this should be good to go

Fil added 3 commits April 26, 2022 14:24
… 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.
@Fil
Copy link
Contributor Author

Fil commented Apr 26, 2022

src/axes.js Outdated
function tickInterval(scale, axis) {
const interval = maybeInterval(scale.interval);
if (interval != null) {
const [min, max] = extent(scale.scale.domain());
Copy link
Member

Choose a reason for hiding this comment

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

This should only be computed if axis.ticks is undefined.

src/axes.js Outdated
if (interval != null) {
const [min, max] = extent(scale.scale.domain());
if (axis.ticks === undefined) axis.ticks = interval.range(interval.floor(min), interval.offset(interval.floor(max)));
if (scale.type !== "point" && scale.type !== "band" && axis.tickFormat === undefined && typeof scale.interval === "number") axis.tickFormat = ",";
Copy link
Member

Choose a reason for hiding this comment

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

This should be using the isBandScale helper probably. Or maybe isOrdinalScale?

More to the point, what is this doing? We shouldn’t assume that a numeric interval is an integer; you can say interval: 0.5 (though maybe fractional intervals are not really recommended—it would probably be better for us to use interval: -2 like we do for d3.tickIncrement). In any case, rather than this default, maybe we should simply default tickFormat to formatDefault so that it works with number and date ticks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is trying to address one specific case: if the domain is [1, 3], the scale linear, the default tickFormat is going to show "1.0", "1.5",… "3.0". If you specify interval: 1, it shows an intent to have integer values, and the ticks should be "1", "2", "3", and not "1.0", "2.0", "3.0". So we're not just indicating that we want integer ticks, but also an integer tick format. Using formatDefault is OK in this case.

However formatDefault fails in the case of years on a utc scale with a d3.utcYear interval: in that case we would prefer to format years as "2001", and not "2001-01-01" (which is what formatDefault does).

Another case that I'm trying to think about is #768, when numbers are in the range [0, 2499] and we prefer to format them as "1999" rather than "1,999".

I'm not sure how to hold all of this together—it seems that we need both the domain's extent and the interval "type" (or maybe the interval should give a preferred tickFormat : utcYear wants to format as "2001", and utcMonth as "2001-01", maybe…).

src/scales.js Outdated
if (options.type === undefined
&& options.domain === undefined
&& options.range === undefined
&& options.interval == null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&& options.interval == null
&& options.interval === undefined

Assuming we do maybeInterval on scale construction (per other comments) we don’t have to worry about null vs. undefined here, too.

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)
@Fil Fil force-pushed the fil/ordinal-interval branch from 390725d to d419c44 Compare June 9, 2022 20:10
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.

I made some changes here:

  1. I changed the default tickFormat for all ordinal scales to be Plot’s default formatter. This way, if you specify numbers or dates as your ordinal ticks (or domain), you’ll get nice localized formatting by default. I figure this shouldn’t depend on whether the scale has an interval or not, and also because I don’t think Plot’s default formatter should override the default tick format on quantitative scales. (Maybe that could change in the future if we have a default tick format that is specific to the interval you’ve specified, but that’s for future.)
  2. I changed the default fontVariant for ordinal scales with an interval to use tabular-nums so that you get better tick labels (just like you would with a quantitative scale).
  3. I moved to interval coercion (maybeInterval call) slightly earlier in scale initialization.
  4. I augmented one of the tests to ensure that the scale transform defaults to interval.floor.
  5. I added a comment to the test saying that using a transform to parse dates is not recommended. (You should do that earlier when you load data.)
  6. I adopted more shorthand in the tests where possible.

@mbostock mbostock merged commit 48166af into mbostock/ordinal-interval Jun 10, 2022
@mbostock mbostock deleted the fil/ordinal-interval branch June 10, 2022 15:35
mbostock added a commit that referenced this pull request Jun 10, 2022
* 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 added a commit that referenced this pull request Jun 10, 2022
* ordinal interval

* fix test
(913629f)

* date scale interval & warning (#852)

* 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]>

* Update README

* options.interval is not normalized here

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