You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/08/25 17:23:24 UTC
[GitHub] [commons-numbers] XenoAmess opened a new pull request #83: [NUMBERS-150] fix bug in Fraction.pow
XenoAmess opened a new pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476713984
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
Review comment:
@aherbert done.
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +775,31 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent > 0) {
+ return new Fraction(ArithmeticUtils.pow(this.numerator, exponent),
Review comment:
@aherbert done.
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +775,31 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent > 0) {
+ return new Fraction(ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,
+ ArithmeticUtils.pow(this.numerator, Integer.MAX_VALUE) * this.numerator);
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ return new Fraction(ArithmeticUtils.pow(this.denominator, -exponent),
+ ArithmeticUtils.pow(this.numerator, -exponent));
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476678386
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
##########
@@ -24,6 +24,8 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476716205
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
##########
@@ -622,6 +622,16 @@ void testMath1261() {
assertFraction(1, Integer.MAX_VALUE, b.divide(2));
}
+ @Test
+ void testNumbers150() {
+ // zero to negative powers should throw an exception
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(-1));
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(Integer.MIN_VALUE));
+
+ // shall overflow
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.of(2).pow(Integer.MIN_VALUE));
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476632088
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
- if (isZero()) {
- return ZERO;
+ if (exponent == -1) {
+ return this.reciprocal();
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (this.denominator == 0) {
+ if (exponent > 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
+ }
+ if (this.numerator == 0) {
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ if (exponent > 0) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent)
+ );
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,
Review comment:
@aherbert
No need to use Math.multipyExact here.
Because:
If this.denominator be 0,1,or -1, then it will never overflow.
Other wise, any other int do pow with Integer.MAX_VALUE will overflow, it will overflow before we do the multiply.
So here is a tricky but correct implement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
aherbert commented on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680218479
Your current code **is** throwing an exception for Fraction(0/1).pow(-1):
```java
if (isZero()) {
if (exponent < 0) {
throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
```
You even have tests to show this. So yes, this is the correct code as it stands.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476718724
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
+ this.denominator.pow(exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ // MIN_VALUE can't be negated
+ final int temp = exponent / 2;
+ final BigInteger newDenominator = denominator.pow(-temp);
+ final BigInteger newNumerator = numerator.pow(-temp);
+ return new BigFraction(newDenominator.multiply(newDenominator),
Review comment:
@aherbert got it.
then I will make it same to what in Fraction.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] coveralls commented on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680232658
[](https://coveralls.io/builds/32995942)
Coverage increased (+0.003%) to 99.635% when pulling **44678423889201f2c60ac024e132593336033bc9 on XenoAmess:fix_bug_in_Fraction_pow** into **3282fb87ebcc905ef1daa6a7ac24740c3f73cf4f on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476715943
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
##########
@@ -778,4 +778,14 @@ void testMath340() {
fractionA.getDenominator().multiply(fractionB.getDenominator()));
Assertions.assertEquals(correctResult, errorResult);
}
+
+ @Test
+ void testNumbers150() {
+ // zero to negative powers should throw an exception
+ Assertions.assertThrows(ArithmeticException.class, () -> BigFraction.ZERO.pow(-1));
+ Assertions.assertThrows(ArithmeticException.class, () -> BigFraction.ZERO.pow(Integer.MIN_VALUE));
+
+ // shall overflow
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.of(2).pow(Integer.MIN_VALUE));
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476679201
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,38 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
- return ZERO;
- }
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
aherbert commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476704947
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
##########
@@ -622,6 +622,16 @@ void testMath1261() {
assertFraction(1, Integer.MAX_VALUE, b.divide(2));
}
+ @Test
+ void testNumbers150() {
+ // zero to negative powers should throw an exception
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(-1));
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(Integer.MIN_VALUE));
+
+ // shall overflow
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.of(2).pow(Integer.MIN_VALUE));
Review comment:
Add the case for 1 / 2 as well.
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
+ this.denominator.pow(exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ // MIN_VALUE can't be negated
+ final int temp = exponent / 2;
+ final BigInteger newDenominator = denominator.pow(-temp);
+ final BigInteger newNumerator = numerator.pow(-temp);
+ return new BigFraction(newDenominator.multiply(newDenominator),
Review comment:
Why not the same as Fraction:
```java
return new Fraction(denominator.pow(Integer.MAX_VALUE).multiply(denominator),
numerator.pow(Integer.MAX_VALUE).multiply(numerator));
```
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +775,31 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent > 0) {
+ return new Fraction(ArithmeticUtils.pow(this.numerator, exponent),
Review comment:
No need for `this` keyword.
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +775,31 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent > 0) {
+ return new Fraction(ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,
+ ArithmeticUtils.pow(this.numerator, Integer.MAX_VALUE) * this.numerator);
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ return new Fraction(ArithmeticUtils.pow(this.denominator, -exponent),
+ ArithmeticUtils.pow(this.numerator, -exponent));
Review comment:
Formatting should match the original with whitespace to align the code between lines.
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
Review comment:
No need to `this` keyword
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
##########
@@ -778,4 +778,14 @@ void testMath340() {
fractionA.getDenominator().multiply(fractionB.getDenominator()));
Assertions.assertEquals(correctResult, errorResult);
}
+
+ @Test
+ void testNumbers150() {
+ // zero to negative powers should throw an exception
+ Assertions.assertThrows(ArithmeticException.class, () -> BigFraction.ZERO.pow(-1));
+ Assertions.assertThrows(ArithmeticException.class, () -> BigFraction.ZERO.pow(Integer.MIN_VALUE));
+
+ // shall overflow
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.of(2).pow(Integer.MIN_VALUE));
Review comment:
I would add `Fraction.of(1, 2)` as well to test overflow in the numerator or denominator
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonTestCases.java
##########
@@ -570,6 +570,7 @@ private CommonTestCases() {}
testCases.add(new BinaryIntOperatorTestCase(0, 1, Integer.MAX_VALUE, 0, 1));
testCases.add(new BinaryIntOperatorTestCase(0, -1, Integer.MAX_VALUE, 0, 1));
+ testCases.add(new BinaryIntOperatorTestCase(1, 1, Integer.MIN_VALUE, 1, 1));
Review comment:
I would add all the cases for completeness:
```
1, 1
1, -1
-1, 1
-1, -1
```
These should all evaluate to 1 / 1.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476677533
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
##########
@@ -778,4 +780,15 @@ void testMath340() {
fractionA.getDenominator().multiply(fractionB.getDenominator()));
Assertions.assertEquals(correctResult, errorResult);
}
+
+ @Test
+ void testNumbers150(){
+ // zero to negative powers should throw an exception
+ assertThrows(ArithmeticException.class, () -> BigFraction.ZERO.pow(-1));
+ assertThrows(ArithmeticException.class, () -> BigFraction.ZERO.pow(Integer.MIN_VALUE));
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680224835
@aherbert
Hi.
I think I fixed all parts you pointed out.
Please do a re-review.
Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] coveralls edited a comment on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680232658
[](https://coveralls.io/builds/32997637)
Coverage increased (+0.002%) to 99.634% when pulling **0fd1da5bdb92bf31b7dd4ec3981924c5f68dbef4 on XenoAmess:fix_bug_in_Fraction_pow** into **3282fb87ebcc905ef1daa6a7ac24740c3f73cf4f on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
aherbert commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476624765
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonsLangPortedFractionTest.java
##########
@@ -0,0 +1,1110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.numbers.fraction;
+
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Test cases for the {@link Fraction} class
+ */
+public class CommonsLangPortedFractionTest {
Review comment:
This class should not be in this PR.
You should update FractionTest to add assertions that fail with the current code. See the comments in the Jira ticket.
A similar change is warranted for BigFraction.
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
- if (isZero()) {
- return ZERO;
+ if (exponent == -1) {
+ return this.reciprocal();
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (this.denominator == 0) {
Review comment:
The private constructor should make it impossible to have a denominator of zero. See if you can write a test that hits this code execution path.
This functionality is better served by the current logic that checks if (isZero()).
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
- if (isZero()) {
- return ZERO;
+ if (exponent == -1) {
+ return this.reciprocal();
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (this.denominator == 0) {
+ if (exponent > 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
+ }
+ if (this.numerator == 0) {
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ if (exponent > 0) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent)
+ );
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,
Review comment:
A raw multiplication here is a bad idea. You should at least use Math.multipyExact. For this extreme edge case I think some variant of the following simpler logic is clearer:
```java
if (exponent == Integer.MIN_VALUE) {
// MIN_VALUE can't be negated
final int temp = exponent / 2;
return new Fraction(ArithmeticUtils.pow(denominator, -temp),
ArithmeticUtils.pow(numerator, -temp)).pow(2);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476632088
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
- if (isZero()) {
- return ZERO;
+ if (exponent == -1) {
+ return this.reciprocal();
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (this.denominator == 0) {
+ if (exponent > 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
+ }
+ if (this.numerator == 0) {
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ if (exponent > 0) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent)
+ );
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,
Review comment:
@aherbert
No need to use Math.multipyExact here.
Because:
If this.denominator be 0,1,or -1, then it will never overflow.
Other wise, any other int do pow with Integer.MAX_VALUE will overflow, it will overflow in ArithmeticUtils.pow, and thus before we do the raw multiply.
So here is a tricky but correct implement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476632088
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
- if (isZero()) {
- return ZERO;
+ if (exponent == -1) {
+ return this.reciprocal();
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (this.denominator == 0) {
+ if (exponent > 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
+ }
+ if (this.numerator == 0) {
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ if (exponent > 0) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent)
+ );
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,
Review comment:
@aherbert
No need to use Math.multipyExact here.
Because:
If this.denominator be 0,1,or -1, then it will never overflow.
Other wise, any other int do pow with Integer.MAX_VALUE will overflow, it will overflow in ArithmeticUtils.pow, and thus before we do the raw multiply.
So it is a tricky but correct implement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess edited a comment on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680216107
@aherbert
So should it throw exception when Fraction(0/1).pow(-1)? Or just return ONE?
If throw, then I should change the codes.
If not throw, then I should add comments, and change the tests.
Please...I'm really confused...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476632088
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
- if (isZero()) {
- return ZERO;
+ if (exponent == -1) {
+ return this.reciprocal();
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (this.denominator == 0) {
+ if (exponent > 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
+ }
+ if (this.numerator == 0) {
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ if (exponent > 0) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent)
+ );
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ return new Fraction(
+ ArithmeticUtils.pow(this.denominator, Integer.MAX_VALUE) * this.denominator,
Review comment:
@aherbert
No need to use ArithmeticUtils.pow here.
Because:
If this.denominator be 0,1,or -1, then it will never overflow.
Other wise, any other int do pow with Integer.MAX_VALUE will overflow, it will overflow before we do the multiply.
So here is a tricky but correct implement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476682533
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
##########
@@ -622,6 +624,17 @@ void testMath1261() {
assertFraction(1, Integer.MAX_VALUE, b.divide(2));
}
+ @Test
+ void testNumbers150(){
+ // zero to negative powers should throw an exception
+ assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(-1));
+ assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(Integer.MIN_VALUE));
Review comment:
@aherbert done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
aherbert commented on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680300714
Added to master in commit a8897d18c8838cece48335554cf6d7e8fc8edc28
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert closed pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
aherbert closed pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680193022
@aherbert
Hi.
done/replied all you suggested.
please re-review.
thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476684725
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +968,46 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
- return ZERO;
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
+ }
+ if (exponent > 0) {
+ return new BigFraction(
+ ArithmeticUtils.pow(this.numerator, exponent),
Review comment:
@aherbert done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476632960
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,45 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
- if (isZero()) {
- return ZERO;
+ if (exponent == -1) {
+ return this.reciprocal();
}
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (this.denominator == 0) {
Review comment:
@aherbert
Yes, you are correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476683287
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +968,46 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
- return ZERO;
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
Review comment:
@aherbert
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +968,46 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
- return ZERO;
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
Review comment:
@aherbert done.
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,38 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
- return ZERO;
- }
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] coveralls edited a comment on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680232658
[](https://coveralls.io/builds/32998301)
Coverage increased (+0.002%) to 99.634% when pulling **0fd1da5bdb92bf31b7dd4ec3981924c5f68dbef4 on XenoAmess:fix_bug_in_Fraction_pow** into **3282fb87ebcc905ef1daa6a7ac24740c3f73cf4f on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476680145
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
##########
@@ -775,19 +778,38 @@ public Fraction divide(Fraction value) {
*/
@Override
public Fraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
- return ZERO;
- }
-
- if (exponent < 0) {
- return new Fraction(ArithmeticUtils.pow(denominator, -exponent),
- ArithmeticUtils.pow(numerator, -exponent));
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
}
- return new Fraction(ArithmeticUtils.pow(numerator, exponent),
- ArithmeticUtils.pow(denominator, exponent));
+ if (exponent > 0) {
+ return new Fraction(
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476716607
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
##########
@@ -622,6 +622,16 @@ void testMath1261() {
assertFraction(1, Integer.MAX_VALUE, b.divide(2));
}
+ @Test
+ void testNumbers150() {
+ // zero to negative powers should throw an exception
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(-1));
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.ZERO.pow(Integer.MIN_VALUE));
+
+ // shall overflow
+ Assertions.assertThrows(ArithmeticException.class, () -> Fraction.of(2).pow(Integer.MIN_VALUE));
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476634642
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonsLangPortedFractionTest.java
##########
@@ -0,0 +1,1110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.numbers.fraction;
+
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Test cases for the {@link Fraction} class
+ */
+public class CommonsLangPortedFractionTest {
Review comment:
@aherbert
removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476717042
##########
File path: commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonTestCases.java
##########
@@ -570,6 +570,7 @@ private CommonTestCases() {}
testCases.add(new BinaryIntOperatorTestCase(0, 1, Integer.MAX_VALUE, 0, 1));
testCases.add(new BinaryIntOperatorTestCase(0, -1, Integer.MAX_VALUE, 0, 1));
+ testCases.add(new BinaryIntOperatorTestCase(1, 1, Integer.MIN_VALUE, 1, 1));
Review comment:
@aherbert done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
aherbert commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476716184
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
+ this.denominator.pow(exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ // MIN_VALUE can't be negated
+ final int temp = exponent / 2;
+ final BigInteger newDenominator = denominator.pow(-temp);
+ final BigInteger newNumerator = numerator.pow(-temp);
+ return new BigFraction(newDenominator.multiply(newDenominator),
Review comment:
It will throw an `ArithmeticException`. This is the same as the code for the positive exponent just with the numerator and denominator reversed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476683956
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +968,46 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
- return ZERO;
+ if (exponent < 0) {
+ throw new ArithmeticException(STRING_THE_DENOMINATOR_MUST_NOT_BE_ZERO);
+ } else {
+ return ZERO;
+ }
+ }
+ if (exponent > 0) {
+ return new BigFraction(
+ ArithmeticUtils.pow(this.numerator, exponent),
+ ArithmeticUtils.pow(this.denominator, exponent)
+ );
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ // MIN_VALUE can't be negated
+ final int temp = exponent >> 1;
Review comment:
@aherbert OK, as it will be optimized by compiler anyway
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on a change in pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#discussion_r476711635
##########
File path: commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
##########
@@ -966,23 +966,39 @@ public BigFraction divide(final BigFraction value) {
*/
@Override
public BigFraction pow(final int exponent) {
+ if (exponent == 1) {
+ return this;
+ }
if (exponent == 0) {
return ONE;
}
if (isZero()) {
+ if (exponent < 0) {
+ throw new FractionException(FractionException.ERROR_ZERO_DENOMINATOR);
+ }
return ZERO;
}
-
+ if (exponent > 0) {
+ return new BigFraction(this.numerator.pow(exponent),
+ this.denominator.pow(exponent));
+ }
+ if (exponent == -1) {
+ return this.reciprocal();
+ }
+ if (exponent == Integer.MIN_VALUE) {
+ // MIN_VALUE can't be negated
+ final int temp = exponent / 2;
+ final BigInteger newDenominator = denominator.pow(-temp);
+ final BigInteger newNumerator = numerator.pow(-temp);
+ return new BigFraction(newDenominator.multiply(newDenominator),
Review comment:
@aherbert because I'm not quite sure if BigInteger will throw the exact Exception type that we need...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] XenoAmess commented on pull request #83: [NUMBERS-150] fix bug in Fraction.pow
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #83:
URL: https://github.com/apache/commons-numbers/pull/83#issuecomment-680216107
@aherbert
So should it throw exception when Fraction(0/1).pow(-1)? Or just return ONE?
If throw, then I should change the codes.
If not throw, then I should add comments, and change the tests.
Please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org