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

Range.BigDecimal and Range.Double misses last element #4985

Closed
scabug opened this issue Sep 9, 2011 · 5 comments
Closed

Range.BigDecimal and Range.Double misses last element #4985

scabug opened this issue Sep 9, 2011 · 5 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Sep 9, 2011

This is fairly simple to reproduce:

Range.Double(0.0, 1.0, 0.3)

Gives NumericRange(0.0, 0.3, 0.6). It should give NumericRange(0.0, 0.3, 0.6, 0.9).

It misses this last element because, for some reason, when calculating the length (in NumericRange.count), the "remainder" (num.rem(diff, step)) is converted to a Long (num.toLong(num.rem(diff, step))) and then compares it to num.zero. Any "remainder" < 1.0 would mean the last element is omitted, since num.toLong would force it to 0, which then compares with num.zero as true. In the above case, the remainder is 0.1, so the problem presents itself.

I've attached a simple patch which removes the num.toLong conversion and fixes the issue.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 9, 2011

Imported From: https://issues.scala-lang.org/browse/SI-4985?orig=1
Reporter: Thomas Switzer (tixxit)
Affected Versions: 2.9.0, 2.9.1, 2.9.2
Attachments:

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 29, 2011

@paulp said:
Have you tested that patch against existing tests? This is a messed up situation right here: the count code is written assuming it's dealing with an integral type, which seems pretty reasonable given that the implicit parameter is Integral, not Numeric. DoubleAsIfIntegral was an ill-conceived idea, I would definitely like to axe that. Anyway, that's why it converts to Long, and it does so in an attempt to recognize overflow. On the presumption there are test cases for the overflow check, you have probably broken those.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 30, 2011

Thomas Switzer (tixxit) said:
Hi Paul, all the tests pass, except for fft.scala, because it uses LESS boxed longs with my patch (I'll attach the patch for fft.check + the patch above so that it passes).

I don't think overflow is an issue (but I may also have misunderstand what you meant). The problem is from the remainder being converted to a Long, not the actual count (which makes sense as a Long). Since the remainder, after being converted to Long, is being compared against num.zero (not 0L), then you are doing (eg.) Double -> Long -> Double so that it can be compared against num.zero. This is also why there are less boxed longs in the FFT test after applying this patch, because w/o it we are needlessly unboxing then reboxing longs so we can compare them to num.zero (num is an Integral[Long] in fft.scala).

The remainder check is only there to handle the case where the right side is excluded and the right side (the end) would have been included had it been inclusive. Converting it to a Long first before comparing it w/ num.zero doesn't have any benefits (and also leads to this bug) and the actual "remainder" variable is only ever used in its comparison with num.zero.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 30, 2011

Thomas Switzer (tixxit) said:
Fix for the bug + fix for the failed fft check.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 31, 2011

Commit Message Bot (anonymous) said:
(extempore in r25918) Fix for NumericRange boundary condition.

Contributed by Thomas Switzer. Closes #4985, no review.

@scabug scabug closed this Oct 31, 2011
@scabug scabug added this to the 2.9.2 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.