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

linear.nice() ticks do not always span the domain #199

Closed
jonathonadams opened this issue Mar 23, 2021 · 2 comments · Fixed by #200
Closed

linear.nice() ticks do not always span the domain #199

jonathonadams opened this issue Mar 23, 2021 · 2 comments · Fixed by #200

Comments

@jonathonadams
Copy link

const scale = d3
  .scaleLinear()
  .domain([0.98, 1.13])
  .nice()

scale.domain() // [0.98, 1.14] as expected
scale.ticks() // [0.98, 1, 1.02, 1.04, 1.06, 1.08, 1.1, 1.12]
scale.ticks(17) // [0.98, 0.99, 1, 1.01, 1.02, 1.03, 1.04, 1.05, 1.06, 1.07, 1.08, 1.09, 1.1, 1.11, 1.12, 1.13]

I believe this issue is a floating point number precision error in the d3-array/ticks

For this specific example and following the bouncing ball, if ticks is called with (0.98, 1.14, 10) then we get the following on line 23

step = -50; // -> 50
start * step = 0.98 * 50 = 49;
end * step = 1.14 * 50 = 56.999999999999;

Naturally calling floor unintentionally reduces the stop value by 1.

PR #164 was added to solve #162 however I think it just inverted the issue.

In this scenario the precision is not important so I think a possible solution is to call toPrecision() or use d3-format?

Happy to submit a PR after discussion.

Note:
This issue is more an issue for d3-array than d3-scale but I have submitted it here as this is where the issue surfaced for me. Happy to close this and open one for d3-array if required.

Edit:
I note that most of the tests covering this are using integer domains and a relatively small count (2,3,4). If I call check([0.98, 1.14], 2/3/4); then they all pass but not with count = 10; (default).

Should this test suit be expanded a little?

Second Edit:
On second thoughts toPrecision wouldn't work due to the unknown size of the start on stop but possible something as simple as the following:

function round (x) {
  return Math.round(x * 100) / 100
}

if (step > 0) {
  start = Math.ceil(round(start / step));
  stop = Math.floor(round(stop / step));
  ticks = new Array(n = Math.ceil(stop - start + 1));
  while (++i < n) ticks[i] = (start + i) * step;
} else {
  step = -step;
  start = Math.ceil(round(start * step));
  stop = Math.floor(round(stop * step));
  ticks = new Array(n = Math.ceil(stop - start + 1));
  while (++i < n) ticks[i] = (start + i) / step;
}

There is possibly a wider issue/effect that I am unaware on but this patch does fix my issue and make all the test pass including that of d3-array.

@mbostock mbostock transferred this issue from d3/d3-scale Mar 23, 2021
@mbostock
Copy link
Member

Using the d3-array API:

d3.nice(0.98, 1.13, 10) // [0.98, 1.14]
d3.ticks(0.98, 1.14, 10) // [0.98, 1, 1.02, 1.04, 1.06, 1.08, 1.1, 1.12]

Rounding to some arbitrary precision is not going to work in general. I think we might need to do something like this instead:

step = -step;
start = Math.ceil(start * step);
const r = Math.round(stop * step);
stop = r - (r / step > stop);

@jonathonadams
Copy link
Author

I was unsure of the wider effect or use so makes sense it's not as simple as rounding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants