You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by luc <lu...@spaceroots.org> on 2015/05/04 16:20:04 UTC
Re: [math] Attempt to circumvent some errors which seem to be platform-dependent.
Le 2015-05-04 14:32, Benedikt Ritter a écrit :
> Hello Luc,
>
> 2015-05-04 13:43 GMT+02:00 <lu...@apache.org>:
>
>> Repository: commons-math
>> Updated Branches:
>> refs/heads/master c8cb75243 -> c771c0080
>>
>>
>> Attempt to circumvent some errors which seem to be platform-dependent.
>>
>> The Jenkins build often fails on code that seems to be perfectly
>> correct. Failures also do no always happen so they may depend on
>> platform. There were similar problems a few months ago that were
>> probably related to JIT bugs.
>>
>> This fix simply tries to do the same thing as before, but with an
>> earlier detection of NaN in one case, and by comparing directly the
>> bits
>> representation in another case, to avoid wrong optimizations.
>>
>> Project: http://git-wip-us.apache.org/repos/asf/commons-math/repo
>> Commit:
>> http://git-wip-us.apache.org/repos/asf/commons-math/commit/c771c008
>> Tree:
>> http://git-wip-us.apache.org/repos/asf/commons-math/tree/c771c008
>> Diff:
>> http://git-wip-us.apache.org/repos/asf/commons-math/diff/c771c008
>>
>> Branch: refs/heads/master
>> Commit: c771c0080b08abd80418c4e88f1be3efec828f0a
>> Parents: c8cb752
>> Author: Luc Maisonobe <lu...@apache.org>
>> Authored: Mon May 4 13:43:27 2015 +0200
>> Committer: Luc Maisonobe <lu...@apache.org>
>> Committed: Mon May 4 13:43:27 2015 +0200
>>
>> ----------------------------------------------------------------------
>> .../org/apache/commons/math4/util/FastMath.java | 28
>> +++++++++-----------
>> .../apache/commons/math4/util/FastMathTest.java | 4 +--
>> 2 files changed, 15 insertions(+), 17 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/commons-math/blob/c771c008/src/main/java/org/apache/commons/math4/util/FastMath.java
>> ----------------------------------------------------------------------
>> diff --git a/src/main/java/org/apache/commons/math4/util/FastMath.java
>> b/src/main/java/org/apache/commons/math4/util/FastMath.java
>> index 24bb857..fcd03ea 100644
>> --- a/src/main/java/org/apache/commons/math4/util/FastMath.java
>> +++ b/src/main/java/org/apache/commons/math4/util/FastMath.java
>> @@ -315,6 +315,9 @@ public class FastMath {
>> /** Mask used to clear the non-sign part of a long. */
>> private static final long MASK_NON_SIGN_LONG =
>> 0x7fffffffffffffffl;
>>
>> + /** Bits representation of +1.0. */
>> + private static final long PLUS_ONE_BITS = 0x3ff0000000000000L;
>> +
>> /** 2^52 - double numbers this large must be integral (no
>> fraction)
>> or NaN or Infinite */
>> private static final double TWO_POWER_52 = 4503599627370496.0;
>> /** 2^53 - double numbers this large must be even. */
>> @@ -1468,6 +1471,10 @@ public class FastMath {
>> return x;
>> }
>>
>> + if (y != y) { // Y is NaN
>>
>
> It really took me some time to understand this change. How about using
> Double.isNaN(double) instead? It does the same as the current code, but
> reads better, IMHO.
I agree but in this huge class this is how all NaNs are detected and
there
are a bunch of such tests. I don't know the reason these existing tests
were done this way and not using Double.isNaN, it may well be
performance related.
So for this class (and this class only), I prefer to do it the same way
it
is already done a few lines above or below.
best regards,
Luc
>
> Best regards,
> Benedikt
>
>
>> + return y;
>> + }
>> +
>> if (x == 0) {
>> long bits = Double.doubleToRawLongBits(x);
>> if ((bits & 0x8000000000000000L) != 0) {
>> @@ -1485,18 +1492,13 @@ public class FastMath {
>>
>> if (y < 0) {
>> return Double.POSITIVE_INFINITY;
>> - }
>> - if (y > 0) {
>> + } else {
>> return 0.0;
>> }
>>
>> - return Double.NaN;
>> }
>>
>> if (x == Double.POSITIVE_INFINITY) {
>> - if (y != y) { // y is NaN
>> - return y;
>> - }
>> if (y < 0.0) {
>> return 0.0;
>> } else {
>> @@ -1505,21 +1507,17 @@ public class FastMath {
>> }
>>
>> if (y == Double.POSITIVE_INFINITY) {
>> - if (x * x == 1.0) {
>> - return Double.NaN;
>> - }
>> -
>> - if (x * x > 1.0) {
>> + long bitsAbsX = MASK_NON_SIGN_LONG &
>> Double.doubleToRawLongBits(x);
>> + if (bitsAbsX > PLUS_ONE_BITS) {
>> return Double.POSITIVE_INFINITY;
>> - } else {
>> + } else if (bitsAbsX < PLUS_ONE_BITS) {
>> return 0.0;
>> + } else {
>> + return Double.NaN;
>> }
>> }
>>
>> if (x == Double.NEGATIVE_INFINITY) {
>> - if (y != y) { // y is NaN
>> - return y;
>> - }
>>
>> if (y < 0) {
>> long yi = (long) y;
>>
>>
>> http://git-wip-us.apache.org/repos/asf/commons-math/blob/c771c008/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>> ----------------------------------------------------------------------
>> diff --git
>> a/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>> b/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>> index 5d36fea..06a1d07 100644
>> --- a/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>> +++ b/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>> @@ -29,8 +29,6 @@ import
>> org.apache.commons.math4.exception.MathArithmeticException;
>> import org.apache.commons.math4.random.MersenneTwister;
>> import org.apache.commons.math4.random.RandomGenerator;
>> import org.apache.commons.math4.random.Well1024a;
>> -import org.apache.commons.math4.util.FastMath;
>> -import org.apache.commons.math4.util.Precision;
>> import org.junit.Assert;
>> import org.junit.Before;
>> import org.junit.Ignore;
>> @@ -393,6 +391,8 @@ public class FastMathTest {
>>
>> Assert.assertTrue("pow(-2.0, 3.5) should be NaN",
>> Double.isNaN(FastMath.pow(-2.0, 3.5)));
>>
>> + Assert.assertTrue("pow(-0.0, NaN) should be NaN",
>> Double.isNaN(FastMath.pow(-0.0, Double.NaN)));
>> +
>> // Added tests for a 100% coverage
>>
>> Assert.assertTrue("pow(+Inf, NaN) should be NaN",
>> Double.isNaN(FastMath.pow(Double.POSITIVE_INFINITY, Double.NaN)));
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Attempt to circumvent some errors which seem to be platform-dependent.
Posted by Luc Maisonobe <lu...@spaceroots.org>.
Le 04/05/2015 21:34, Benedikt Ritter a écrit :
> 2015-05-04 16:20 GMT+02:00 luc <lu...@spaceroots.org>:
>
>> Le 2015-05-04 14:32, Benedikt Ritter a écrit :
>>
>>> Hello Luc,
>>>
>>> 2015-05-04 13:43 GMT+02:00 <lu...@apache.org>:
>>>
>>> Repository: commons-math
>>>> Updated Branches:
>>>> refs/heads/master c8cb75243 -> c771c0080
>>>>
>>>>
>>>> Attempt to circumvent some errors which seem to be platform-dependent.
>>>>
>>>> The Jenkins build often fails on code that seems to be perfectly
>>>> correct. Failures also do no always happen so they may depend on
>>>> platform. There were similar problems a few months ago that were
>>>> probably related to JIT bugs.
>>>>
>>>> This fix simply tries to do the same thing as before, but with an
>>>> earlier detection of NaN in one case, and by comparing directly the bits
>>>> representation in another case, to avoid wrong optimizations.
>>>>
>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-math/repo
>>>> Commit:
>>>> http://git-wip-us.apache.org/repos/asf/commons-math/commit/c771c008
>>>> Tree: http://git-wip-us.apache.org/repos/asf/commons-math/tree/c771c008
>>>> Diff: http://git-wip-us.apache.org/repos/asf/commons-math/diff/c771c008
>>>>
>>>> Branch: refs/heads/master
>>>> Commit: c771c0080b08abd80418c4e88f1be3efec828f0a
>>>> Parents: c8cb752
>>>> Author: Luc Maisonobe <lu...@apache.org>
>>>> Authored: Mon May 4 13:43:27 2015 +0200
>>>> Committer: Luc Maisonobe <lu...@apache.org>
>>>> Committed: Mon May 4 13:43:27 2015 +0200
>>>>
>>>> ----------------------------------------------------------------------
>>>> .../org/apache/commons/math4/util/FastMath.java | 28
>>>> +++++++++-----------
>>>> .../apache/commons/math4/util/FastMathTest.java | 4 +--
>>>> 2 files changed, 15 insertions(+), 17 deletions(-)
>>>> ----------------------------------------------------------------------
>>>>
>>>>
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/commons-math/blob/c771c008/src/main/java/org/apache/commons/math4/util/FastMath.java
>>>> ----------------------------------------------------------------------
>>>> diff --git a/src/main/java/org/apache/commons/math4/util/FastMath.java
>>>> b/src/main/java/org/apache/commons/math4/util/FastMath.java
>>>> index 24bb857..fcd03ea 100644
>>>> --- a/src/main/java/org/apache/commons/math4/util/FastMath.java
>>>> +++ b/src/main/java/org/apache/commons/math4/util/FastMath.java
>>>> @@ -315,6 +315,9 @@ public class FastMath {
>>>> /** Mask used to clear the non-sign part of a long. */
>>>> private static final long MASK_NON_SIGN_LONG = 0x7fffffffffffffffl;
>>>>
>>>> + /** Bits representation of +1.0. */
>>>> + private static final long PLUS_ONE_BITS = 0x3ff0000000000000L;
>>>> +
>>>> /** 2^52 - double numbers this large must be integral (no fraction)
>>>> or NaN or Infinite */
>>>> private static final double TWO_POWER_52 = 4503599627370496.0;
>>>> /** 2^53 - double numbers this large must be even. */
>>>> @@ -1468,6 +1471,10 @@ public class FastMath {
>>>> return x;
>>>> }
>>>>
>>>> + if (y != y) { // Y is NaN
>>>>
>>>>
>>> It really took me some time to understand this change. How about using
>>> Double.isNaN(double) instead? It does the same as the current code, but
>>> reads better, IMHO.
>>>
>>
>> I agree but in this huge class this is how all NaNs are detected and there
>> are a bunch of such tests. I don't know the reason these existing tests
>> were done this way and not using Double.isNaN, it may well be performance
>> related.
>> So for this class (and this class only), I prefer to do it the same way it
>> is already done a few lines above or below.
>>
>
> Yes at first I also thought it has something to do with performance. But
> then I looked at the implementation of Double.isNaN(double):
>
> static public boolean isNaN(double v) {
> return (v != v);
> }
>
> So it's probably because of consistency with the rest of the class. Would
> you be willing to merge a PR that changes the whole class to use
> Double.isNaN(double) if I provide one?
You are a committer, you can merge it if you want ;-)
I agree it would be cleaner.
best regards,
Luc
>
> Benedikt
>
>>
>>
>> best regards,
>> Luc
>>
>>
>>
>>
>>> Best regards,
>>> Benedikt
>>>
>>>
>>> + return y;
>>>> + }
>>>> +
>>>> if (x == 0) {
>>>> long bits = Double.doubleToRawLongBits(x);
>>>> if ((bits & 0x8000000000000000L) != 0) {
>>>> @@ -1485,18 +1492,13 @@ public class FastMath {
>>>>
>>>> if (y < 0) {
>>>> return Double.POSITIVE_INFINITY;
>>>> - }
>>>> - if (y > 0) {
>>>> + } else {
>>>> return 0.0;
>>>> }
>>>>
>>>> - return Double.NaN;
>>>> }
>>>>
>>>> if (x == Double.POSITIVE_INFINITY) {
>>>> - if (y != y) { // y is NaN
>>>> - return y;
>>>> - }
>>>> if (y < 0.0) {
>>>> return 0.0;
>>>> } else {
>>>> @@ -1505,21 +1507,17 @@ public class FastMath {
>>>> }
>>>>
>>>> if (y == Double.POSITIVE_INFINITY) {
>>>> - if (x * x == 1.0) {
>>>> - return Double.NaN;
>>>> - }
>>>> -
>>>> - if (x * x > 1.0) {
>>>> + long bitsAbsX = MASK_NON_SIGN_LONG &
>>>> Double.doubleToRawLongBits(x);
>>>> + if (bitsAbsX > PLUS_ONE_BITS) {
>>>> return Double.POSITIVE_INFINITY;
>>>> - } else {
>>>> + } else if (bitsAbsX < PLUS_ONE_BITS) {
>>>> return 0.0;
>>>> + } else {
>>>> + return Double.NaN;
>>>> }
>>>> }
>>>>
>>>> if (x == Double.NEGATIVE_INFINITY) {
>>>> - if (y != y) { // y is NaN
>>>> - return y;
>>>> - }
>>>>
>>>> if (y < 0) {
>>>> long yi = (long) y;
>>>>
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/commons-math/blob/c771c008/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>>> ----------------------------------------------------------------------
>>>> diff --git
>>>> a/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>>> b/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>>> index 5d36fea..06a1d07 100644
>>>> --- a/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>>> +++ b/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>>> @@ -29,8 +29,6 @@ import
>>>> org.apache.commons.math4.exception.MathArithmeticException;
>>>> import org.apache.commons.math4.random.MersenneTwister;
>>>> import org.apache.commons.math4.random.RandomGenerator;
>>>> import org.apache.commons.math4.random.Well1024a;
>>>> -import org.apache.commons.math4.util.FastMath;
>>>> -import org.apache.commons.math4.util.Precision;
>>>> import org.junit.Assert;
>>>> import org.junit.Before;
>>>> import org.junit.Ignore;
>>>> @@ -393,6 +391,8 @@ public class FastMathTest {
>>>>
>>>> Assert.assertTrue("pow(-2.0, 3.5) should be NaN",
>>>> Double.isNaN(FastMath.pow(-2.0, 3.5)));
>>>>
>>>> + Assert.assertTrue("pow(-0.0, NaN) should be NaN",
>>>> Double.isNaN(FastMath.pow(-0.0, Double.NaN)));
>>>> +
>>>> // Added tests for a 100% coverage
>>>>
>>>> Assert.assertTrue("pow(+Inf, NaN) should be NaN",
>>>> Double.isNaN(FastMath.pow(Double.POSITIVE_INFINITY, Double.NaN)));
>>>>
>>>>
>>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Attempt to circumvent some errors which seem to be platform-dependent.
Posted by Benedikt Ritter <br...@apache.org>.
2015-05-04 16:20 GMT+02:00 luc <lu...@spaceroots.org>:
> Le 2015-05-04 14:32, Benedikt Ritter a écrit :
>
>> Hello Luc,
>>
>> 2015-05-04 13:43 GMT+02:00 <lu...@apache.org>:
>>
>> Repository: commons-math
>>> Updated Branches:
>>> refs/heads/master c8cb75243 -> c771c0080
>>>
>>>
>>> Attempt to circumvent some errors which seem to be platform-dependent.
>>>
>>> The Jenkins build often fails on code that seems to be perfectly
>>> correct. Failures also do no always happen so they may depend on
>>> platform. There were similar problems a few months ago that were
>>> probably related to JIT bugs.
>>>
>>> This fix simply tries to do the same thing as before, but with an
>>> earlier detection of NaN in one case, and by comparing directly the bits
>>> representation in another case, to avoid wrong optimizations.
>>>
>>> Project: http://git-wip-us.apache.org/repos/asf/commons-math/repo
>>> Commit:
>>> http://git-wip-us.apache.org/repos/asf/commons-math/commit/c771c008
>>> Tree: http://git-wip-us.apache.org/repos/asf/commons-math/tree/c771c008
>>> Diff: http://git-wip-us.apache.org/repos/asf/commons-math/diff/c771c008
>>>
>>> Branch: refs/heads/master
>>> Commit: c771c0080b08abd80418c4e88f1be3efec828f0a
>>> Parents: c8cb752
>>> Author: Luc Maisonobe <lu...@apache.org>
>>> Authored: Mon May 4 13:43:27 2015 +0200
>>> Committer: Luc Maisonobe <lu...@apache.org>
>>> Committed: Mon May 4 13:43:27 2015 +0200
>>>
>>> ----------------------------------------------------------------------
>>> .../org/apache/commons/math4/util/FastMath.java | 28
>>> +++++++++-----------
>>> .../apache/commons/math4/util/FastMathTest.java | 4 +--
>>> 2 files changed, 15 insertions(+), 17 deletions(-)
>>> ----------------------------------------------------------------------
>>>
>>>
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/commons-math/blob/c771c008/src/main/java/org/apache/commons/math4/util/FastMath.java
>>> ----------------------------------------------------------------------
>>> diff --git a/src/main/java/org/apache/commons/math4/util/FastMath.java
>>> b/src/main/java/org/apache/commons/math4/util/FastMath.java
>>> index 24bb857..fcd03ea 100644
>>> --- a/src/main/java/org/apache/commons/math4/util/FastMath.java
>>> +++ b/src/main/java/org/apache/commons/math4/util/FastMath.java
>>> @@ -315,6 +315,9 @@ public class FastMath {
>>> /** Mask used to clear the non-sign part of a long. */
>>> private static final long MASK_NON_SIGN_LONG = 0x7fffffffffffffffl;
>>>
>>> + /** Bits representation of +1.0. */
>>> + private static final long PLUS_ONE_BITS = 0x3ff0000000000000L;
>>> +
>>> /** 2^52 - double numbers this large must be integral (no fraction)
>>> or NaN or Infinite */
>>> private static final double TWO_POWER_52 = 4503599627370496.0;
>>> /** 2^53 - double numbers this large must be even. */
>>> @@ -1468,6 +1471,10 @@ public class FastMath {
>>> return x;
>>> }
>>>
>>> + if (y != y) { // Y is NaN
>>>
>>>
>> It really took me some time to understand this change. How about using
>> Double.isNaN(double) instead? It does the same as the current code, but
>> reads better, IMHO.
>>
>
> I agree but in this huge class this is how all NaNs are detected and there
> are a bunch of such tests. I don't know the reason these existing tests
> were done this way and not using Double.isNaN, it may well be performance
> related.
> So for this class (and this class only), I prefer to do it the same way it
> is already done a few lines above or below.
>
Yes at first I also thought it has something to do with performance. But
then I looked at the implementation of Double.isNaN(double):
static public boolean isNaN(double v) {
return (v != v);
}
So it's probably because of consistency with the rest of the class. Would
you be willing to merge a PR that changes the whole class to use
Double.isNaN(double) if I provide one?
Benedikt
>
>
> best regards,
> Luc
>
>
>
>
>> Best regards,
>> Benedikt
>>
>>
>> + return y;
>>> + }
>>> +
>>> if (x == 0) {
>>> long bits = Double.doubleToRawLongBits(x);
>>> if ((bits & 0x8000000000000000L) != 0) {
>>> @@ -1485,18 +1492,13 @@ public class FastMath {
>>>
>>> if (y < 0) {
>>> return Double.POSITIVE_INFINITY;
>>> - }
>>> - if (y > 0) {
>>> + } else {
>>> return 0.0;
>>> }
>>>
>>> - return Double.NaN;
>>> }
>>>
>>> if (x == Double.POSITIVE_INFINITY) {
>>> - if (y != y) { // y is NaN
>>> - return y;
>>> - }
>>> if (y < 0.0) {
>>> return 0.0;
>>> } else {
>>> @@ -1505,21 +1507,17 @@ public class FastMath {
>>> }
>>>
>>> if (y == Double.POSITIVE_INFINITY) {
>>> - if (x * x == 1.0) {
>>> - return Double.NaN;
>>> - }
>>> -
>>> - if (x * x > 1.0) {
>>> + long bitsAbsX = MASK_NON_SIGN_LONG &
>>> Double.doubleToRawLongBits(x);
>>> + if (bitsAbsX > PLUS_ONE_BITS) {
>>> return Double.POSITIVE_INFINITY;
>>> - } else {
>>> + } else if (bitsAbsX < PLUS_ONE_BITS) {
>>> return 0.0;
>>> + } else {
>>> + return Double.NaN;
>>> }
>>> }
>>>
>>> if (x == Double.NEGATIVE_INFINITY) {
>>> - if (y != y) { // y is NaN
>>> - return y;
>>> - }
>>>
>>> if (y < 0) {
>>> long yi = (long) y;
>>>
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/commons-math/blob/c771c008/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>> ----------------------------------------------------------------------
>>> diff --git
>>> a/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>> b/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>> index 5d36fea..06a1d07 100644
>>> --- a/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>> +++ b/src/test/java/org/apache/commons/math4/util/FastMathTest.java
>>> @@ -29,8 +29,6 @@ import
>>> org.apache.commons.math4.exception.MathArithmeticException;
>>> import org.apache.commons.math4.random.MersenneTwister;
>>> import org.apache.commons.math4.random.RandomGenerator;
>>> import org.apache.commons.math4.random.Well1024a;
>>> -import org.apache.commons.math4.util.FastMath;
>>> -import org.apache.commons.math4.util.Precision;
>>> import org.junit.Assert;
>>> import org.junit.Before;
>>> import org.junit.Ignore;
>>> @@ -393,6 +391,8 @@ public class FastMathTest {
>>>
>>> Assert.assertTrue("pow(-2.0, 3.5) should be NaN",
>>> Double.isNaN(FastMath.pow(-2.0, 3.5)));
>>>
>>> + Assert.assertTrue("pow(-0.0, NaN) should be NaN",
>>> Double.isNaN(FastMath.pow(-0.0, Double.NaN)));
>>> +
>>> // Added tests for a 100% coverage
>>>
>>> Assert.assertTrue("pow(+Inf, NaN) should be NaN",
>>> Double.isNaN(FastMath.pow(Double.POSITIVE_INFINITY, Double.NaN)));
>>>
>>>
>>>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter