Scala Programming Language
  1. Scala Programming Language
  2. SI-4985

Range.BigDecimal and Range.Double misses last element

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Scala 2.9.0, Scala 2.9.1, Scala 2.9.2
    • Fix Version/s: Scala 2.9.2
    • Component/s: Collections
    • Labels:
      None
    • Environment:

      Any

      Description

      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.

      1. numericrange-count.patch
        0.6 kB
        Thomas Switzer
      2. numeric-range-remainder.patch
        0.9 kB
        Thomas Switzer

        Activity

        Hide
        Paul Phillips added a comment -

        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.

        Show
        Paul Phillips added a comment - 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.
        Hide
        Thomas Switzer added a comment -

        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.

        Show
        Thomas Switzer added a comment - 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.
        Hide
        Thomas Switzer added a comment -

        Fix for the bug + fix for the failed fft check.

        Show
        Thomas Switzer added a comment - Fix for the bug + fix for the failed fft check.
        Hide
        Commit Message Bot added a comment -

        (extempore in r25918) Fix for NumericRange boundary condition.

        Contributed by Thomas Switzer. Closes SI-4985, no review.

        Show
        Commit Message Bot added a comment - (extempore in r25918 ) Fix for NumericRange boundary condition. Contributed by Thomas Switzer. Closes SI-4985 , no review.

          People

          • Assignee:
            Paul Phillips
            Reporter:
            Thomas Switzer
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development