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

Iterator range maintenance #1308

Merged
merged 1 commit into from
May 6, 2016
Merged

Iterator range maintenance #1308

merged 1 commit into from
May 6, 2016

Conversation

l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Apr 28, 2016

Fixes #1309

}
};
}
final double toInclusive = (step > 0) ? Math.nextDown(toExclusive) : Math.nextUp(toExclusive);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make it inclusive by incrementing it with a ULP

Note: this will make exclusive infinities not possible (had to modify a test for that), but I think an exclusive equality to infinity is strange anyway

@danieldietrich
Copy link
Contributor

As described in #1309 these methods should work exactly as in Scala, especially the corner cases (Infinity, NaN, ...).

}
}

// TODO these two need a place to crash for the night :/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danieldietrich, these guys need to sleep somewhere

@codecov-io
Copy link

codecov-io commented Apr 30, 2016

Current coverage is 95.47%

Merging #1308 into master will decrease coverage by -<.01%

  1. 2 files (not in diff) in ...javaslang/concurrent were modified. more
    • Misses +2
    • Hits -2
  2. 7 files (not in diff) in ...javaslang/collection were modified. more
    • Misses +6
    • Partials -1
    • Hits -7
  3. 3 files (not in diff) in .../main/java/javaslang were modified. more
    • Hits -1
@@             master      #1308   diff @@
==========================================
  Files            83         83          
  Lines          9465       9442    -23   
  Methods           0          0          
  Messages          0          0          
  Branches       1712       1709     -3   
==========================================
- Hits           9044       9014    -30   
- Misses          311        319     +8   
+ Partials        110        109     -1   

Powered by Codecov. Last updated by 24ca0a8...185db48

return rangeBy(fromDecimal, toDecimal, stepDecimal);
}

static Iterator<Double> rangeBy(BigDecimal from, BigDecimal toExclusive, BigDecimal step) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would expect an Iterator<BigDecimal> as result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess my helper methods aren't the only one who don't know where to sleep :p

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, no sleep here. topics are too interesting :)

@danieldietrich
Copy link
Contributor

Interesting observation regarding that Scala's double range is backed by Double (is it?):

  val r = 0d to 4d by 0.1                         //> r  : scala.collection.immutable.NumericRange[Double] = NumericRange(0.0, 0.1,
                                                  //|  0.2, 0.30000000000000004, 0.4, 0.5, 0.6, 0.7, 0.7999999999999999, 0.89999999
                                                  //| 99999999, 0.9999999999999999, 1.0999999999999999, 1.2, 1.3, 1.400000000000000
                                                  //| 1, 1.5000000000000002, 1.6000000000000003, 1.7000000000000004, 1.800000000000
                                                  //| 0005, 1.9000000000000006, 2.0000000000000004, 2.1000000000000005, 2.200000000
                                                  //| 0000006, 2.3000000000000007, 2.400000000000001, 2.500000000000001, 2.60000000
                                                  //| 0000001, 2.700000000000001, 2.800000000000001, 2.9000000000000012, 3.00000000
                                                  //| 00000013, 3.1000000000000014, 3.2000000000000015, 3.3000000000000016, 3.40000
                                                  //| 00000000017, 3.5000000000000018, 3.600000000000002, 3.700000000000002, 3.8000
                                                  //| 00000000002, 3.900000000000002, 4.000000000000002)

    import java.math.BigDecimal

  var list: List[Any] = List()                    //> list  : List[Any] = List()
  var b = BigDecimal.valueOf(0d)                  //> b  : java.math.BigDecimal = 0.0
    val step = BigDecimal.valueOf(0.1d)       //> step  : java.math.BigDecimal = 0.1
  while (b.doubleValue() <= 4d) {
      list = list :+ b.doubleValue()
      b = b.add(step)
  }
  println(list)                                   //> List(0.0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0, 1.1, 1.2, 1.3, 1
                                                  //| .4, 1.5, 1.6, 1.7, 1.8, 1.9, 2.0, 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 2.8, 2.
                                                  //| 9, 3.0, 3.1, 3.2, 3.3, 3.4, 3.5, 3.6, 3.7, 3.8, 3.9, 4.0)

@l0rinc
Copy link
Contributor Author

l0rinc commented May 1, 2016

Scala's double range is backed by Double

Quoting from Range.scala:

  // Double works by using a BigDecimal under the hood for precise
  // stepping, but mapping the sequence values back to doubles with
  // .doubleValue.  This constructs the BigDecimals by way of the
  // String constructor (valueOf) instead of the Double one, which
  // is necessary to keep 0.3d at 0.3 as opposed to
  // 0.299999999999999988897769753748434595763683319091796875 or so.

@@ -38,6 +42,42 @@
* @since 2.0.0
*/
public interface Iterator<T> extends java.util.Iterator<T>, Traversable<T> {
/* TODO these don't belong here! */
Copy link
Contributor

@danieldietrich danieldietrich May 1, 2016

Choose a reason for hiding this comment

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

Let's move the BigDecimal helper methods down into the IteratorModule like this (we need to add some static imports for areEqual and asDecimal). Please take a look at my TODO comment below.

    final class BigDecimalHelper {

        private static final Lazy<BigDecimal> INFINITY_DISTANCE = Lazy.of(() -> {
            final BigDecimal two = BigDecimal.valueOf(2);
            final BigDecimal supremum = BigDecimal.valueOf(Math.nextDown(Double.POSITIVE_INFINITY));
            BigDecimal lowerBound = supremum;
            BigDecimal upperBound = two.pow(Double.MAX_EXPONENT + 1);
            while (true) {
                final BigDecimal magicValue = lowerBound.add(upperBound).divide(two, HALF_UP);
                if (Double.isInfinite(magicValue.doubleValue())) {
                    // TODO: or is areEqual(magicValue, upperBound) more stable?
                    if (magicValue.equals(upperBound)) {
                        return magicValue.subtract(supremum);
                    }
                    upperBound = magicValue;
                } else {
                    lowerBound = magicValue;
                }
            }
        });

        /* scale-independent equality */
        static boolean areEqual(BigDecimal from, BigDecimal toExclusive) {
            return from.compareTo(toExclusive) == 0;
        }

        /* parse infinite values also */
        static BigDecimal asDecimal(double number) {
            if (number == NEGATIVE_INFINITY) {
                final BigDecimal result = BigDecimal.valueOf(Math.nextUp(NEGATIVE_INFINITY));
                return result.subtract(INFINITY_DISTANCE.get());
            } else if (number == POSITIVE_INFINITY) {
                final BigDecimal result = BigDecimal.valueOf(Math.nextDown(POSITIVE_INFINITY));
                return result.add(INFINITY_DISTANCE.get());
            } else {
                return BigDecimal.valueOf(number);
            }
        }
    }

For now we do not want to expose these helpers to other classes. Because Java has no module concept (yet), the auxiliary classes (*Module) ensure that they are only accessed from within a class file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

Copy link
Contributor Author

@l0rinc l0rinc May 2, 2016

Choose a reason for hiding this comment

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

Though I'm not sure how to test it now, as it generates a warning :/

@Test
public void infinityAndBeyond() {
    final BigDecimal negativeInfinity = asDecimal(Double.NEGATIVE_INFINITY);
    assertThat(negativeInfinity.add(BigDecimal.ONE).doubleValue()).isNotEqualTo(Double.NEGATIVE_INFINITY);
    assertThat(negativeInfinity.doubleValue()).isEqualTo(Double.NEGATIVE_INFINITY);

    final BigDecimal positiveInfinity = asDecimal(Double.POSITIVE_INFINITY);
    assertThat(positiveInfinity.subtract(BigDecimal.ONE).doubleValue()).isNotEqualTo(Double.POSITIVE_INFINITY);
    assertThat(positiveInfinity.doubleValue()).isEqualTo(Double.POSITIVE_INFINITY);
}

* Changed `double` range to be backed by `BigDecimal` instead
@danieldietrich
Copy link
Contributor

I think this PR is also ready now to be pulled in. I will get a last look at the source tomorrow...

@danieldietrich
Copy link
Contributor

Thanks!

@danieldietrich danieldietrich merged commit e9ce470 into vavr-io:master May 6, 2016
@l0rinc l0rinc deleted the Iterator branch May 6, 2016 23:10
@danieldietrich danieldietrich added this to the 2.1.0 milestone Aug 19, 2016
@danieldietrich danieldietrich modified the milestones: 2.0.5, 2.1.0 Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants