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

Revert fancy range behavior. #5

Closed
andreasplesch opened this issue Aug 18, 2015 · 8 comments
Closed

Revert fancy range behavior. #5

andreasplesch opened this issue Aug 18, 2015 · 8 comments

Comments

@andreasplesch
Copy link
Contributor

While the new range() function is improved, it still produces an extra element for some fractional step sizes.

 range(0, 1, 1/49).length;
50

Also see d3/d3#2524

A fix may have to take into account limited floating point precision, in a similar way to pull request
d3/d3#2526

The problem is

(1-0)/(1/49)
49.00000000000001

which is upped to 50 by Math.ceil().

@andreasplesch
Copy link
Contributor Author

The workaround used in d3/d3#2524 still applies:

eps=1e-15;
range(0, 64).forEach(function(m){rmax=Math.pow(2,m);
  range(1, 1000).forEach( function(d) {rmax=Math.pow(2,m);
  if (range(0, rmax*(1-eps), rmax/d).length !== d) {console.log(d) ;}})})

does not find mismatches in firefox; still need to test in chrome.

This suggests a similar fix, eg. adjust stop by (1-eps) * stop .

Thinking a bit more about it, it is probably necessary to adjust the difference between start and stop, rather than just stop; eg. use (stop - start)*(1-eps) to determine n.

@andreasplesch
Copy link
Contributor Author

Here is a gist which does some checks of the range() function.

http://bl.ocks.org/andreasplesch/ada4b4e89738d4e5d4af

It turns out the workaround is useful but not very good for larger offsets (larger numbers) even when using the (stop-start)*(1-eps) adjustment .

@andreasplesch
Copy link
Contributor Author

Well, I decided to go back to integer ranges.

Perhaps it is worthwhile to add to the documentation that if the number of elements in the generated array is expected to be exactly n, then one should use something like

// var step = (stop - start)/n;
var step = mystep;
d3.array(0, n).map( function(d) {return start + d * step;};

or perhaps provide this as another range function, say d3.nrange(), which takes start, step and number of elements n.

andreasplesch added a commit to andreasplesch/d3-arrays that referenced this issue Aug 19, 2015
Illustrate floating point precision issue with example range and suggest using .map on integer range instead. See issue d3#5 .
@mbostock
Copy link
Member

Hmm. I’m tempted to remove the scale hack entirely, since it only works for some cases and makes the behavior harder to understand.

In general, you shouldn’t expect range(0, 1, 1/n) to return exactly n results because of the limitations of floating point. Using range(0, n).map(function(i) { return i / n; }) is recommended, and I’m in favor of documenting this recommendation.

@mbostock
Copy link
Member

Related d3/d3#494, D3 version 2.7.5, commit d3/d3@87f48b7.

@andreasplesch
Copy link
Contributor Author

Perhaps with the original commit above I now can understand what the conversion to integer is about but it did make the behaviour more opaque. I would say remove it, and perhaps emphasize the limitations a bit more in the documentation.
It took me a little to figure out that the range() behaviour was the issue when I originally used it for
http://bl.ocks.org/andreasplesch/49b2130b15425c1eebc0/
and got some weird results.
So documentation would be great. I tried to keep the additions in my pull request #6 concise but feel free of course to edit as you feel appropriate. I left the start value in there since for some it may not be too trivial to have seen in an example.

@mbostock mbostock reopened this Oct 22, 2015
@mbostock mbostock changed the title range() produces extra element for some fractional steps Revert fancy range behavior. Oct 22, 2015
@mbostock
Copy link
Member

Re-opening this request, because I think you are right that we should remove the fancy behavior.

@mbostock
Copy link
Member

mbostock commented Nov 7, 2015

Fixed in 0.1.2; see d2364d4.

@mbostock mbostock closed this as completed Nov 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants