commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dennis E. Hamilton" <dennis.hamil...@acm.org>
Subject RE: [5/7] [math] Fix "FastMath#round(..)" to comply to changed contract of "Math#round()" in Java 8
Date Sat, 06 Aug 2016 18:46:01 GMT


> -----Original Message-----
> From: Gilles [mailto:gilles@harfang.homelinux.org]
> Sent: Friday, August 5, 2016 09:11
> To: Commons Developers List <dev@commons.apache.org>
> Subject: Re: [5/7] [math] Fix "FastMath#round(..)" to comply to changed
> contract of "Math#round()" in Java 8
> 
> Hi.
> 
> Changes in this file seem unrelated to the commit message.
> 
> Gilles
[orcmid] 

Yes, this appears to be a massive change involving what may largely be stylistic matters (if
that is what Assert.assertMumble << assertMumble is all about).  On first glance, it
makes an excessive change that is extremely difficult to comprehend and review.

What are the practices around expecting small focused single-purpose changes and, in this
case, a small change that addresses the stated purpose of the commit with regard to the contract
for FastMath.round() and its testing?

Is this [VETO] territory, and would the committer be asked to revert the change and break
it down?

I ask this because I don't understand the Commons culture and practices in order to know.

Oh, and is there a JIRA about this (the Math.round contract) or some other way to tie in whatever
scholarship and traceability is involved in the specialized case of [MATH] software?  (I may
have missed this - sorry for asking if already answered.)

 - Dennis


> 
> On Fri, 05 Aug 2016 15:06:53 -0000, ebourg@apache.org wrote:
> >
> > http://git-wip-us.apache.org/repos/asf/commons-
> math/blob/83b70a37/src/test/java/org/apache/commons/math4/util/FastMathT
> est.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 cc95c9c..1f94352 100644
> > --- a/src/test/java/org/apache/commons/math4/util/FastMathTest.java
> > +++ b/src/test/java/org/apache/commons/math4/util/FastMathTest.java
> > @@ -16,6 +16,10 @@
> >   */
> >  package org.apache.commons.math4.util;
> >
> > +import static org.junit.Assert.assertEquals;
> > +import static org.junit.Assert.assertTrue;
> > +import static org.junit.Assert.fail;
> > +
> >  import java.lang.reflect.Method;
> >  import java.lang.reflect.Modifier;
> >  import java.lang.reflect.Type;
> > @@ -30,8 +34,6 @@ import org.apache.commons.math4.dfp.DfpMath;
> >  import org.apache.commons.math4.exception.MathArithmeticException;
> >  import org.apache.commons.math4.rng.UniformRandomProvider;
> >  import org.apache.commons.math4.rng.RandomSource;
> > -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;
> > @@ -42,7 +44,6 @@ public class FastMathTest {
> >      private static final double MAX_ERROR_ULP = 0.51;
> >      private static final int NUMBER_OF_TRIALS = 1000;
> >
> > -
> >      private DfpField field;
> >      private UniformRandomProvider generator;
> >
> > @@ -67,22 +68,22 @@ public class FastMathTest {
> >              { Precision.SAFE_MIN, Precision.EPSILON }
> >          };
> >          for (double[] pair : pairs) {
> > -            Assert.assertEquals("min(" + pair[0] + ", " + pair[1] +
> > ")",
> > -                                Math.min(pair[0], pair[1]),
> > -                                FastMath.min(pair[0], pair[1]),
> > -                                Precision.EPSILON);
> > -            Assert.assertEquals("min(" + pair[1] + ", " + pair[0] +
> > ")",
> > -                                Math.min(pair[1], pair[0]),
> > -                                FastMath.min(pair[1], pair[0]),
> > -                                Precision.EPSILON);
> > -            Assert.assertEquals("max(" + pair[0] + ", " + pair[1] +
> > ")",
> > -                                Math.max(pair[0], pair[1]),
> > -                                FastMath.max(pair[0], pair[1]),
> > -                                Precision.EPSILON);
> > -            Assert.assertEquals("max(" + pair[1] + ", " + pair[0] +
> > ")",
> > -                                Math.max(pair[1], pair[0]),
> > -                                FastMath.max(pair[1], pair[0]),
> > -                                Precision.EPSILON);
> > +            assertEquals("min(" + pair[0] + ", " + pair[1] + ")",
> > +                         Math.min(pair[0], pair[1]),
> > +                         FastMath.min(pair[0], pair[1]),
> > +                         Precision.EPSILON);
> > +            assertEquals("min(" + pair[1] + ", " + pair[0] + ")",
> > +                         Math.min(pair[1], pair[0]),
> > +                         FastMath.min(pair[1], pair[0]),
> > +                         Precision.EPSILON);
> > +            assertEquals("max(" + pair[0] + ", " + pair[1] + ")",
> > +                         Math.max(pair[0], pair[1]),
> > +                         FastMath.max(pair[0], pair[1]),
> > +                         Precision.EPSILON);
> > +            assertEquals("max(" + pair[1] + ", " + pair[0] + ")",
> > +                         Math.max(pair[1], pair[0]),
> > +                         FastMath.max(pair[1], pair[0]),
> > +                         Precision.EPSILON);
> >          }
> >      }
> >
> > @@ -100,39 +101,39 @@ public class FastMathTest {
> >              {  Float.NaN, Float.POSITIVE_INFINITY }
> >          };
> >          for (float[] pair : pairs) {
> > -            Assert.assertEquals("min(" + pair[0] + ", " + pair[1] +
> > ")",
> > -                                Math.min(pair[0], pair[1]),
> > -                                FastMath.min(pair[0], pair[1]),
> > -                                Precision.EPSILON);
> > -            Assert.assertEquals("min(" + pair[1] + ", " + pair[0] +
> > ")",
> > -                                Math.min(pair[1], pair[0]),
> > -                                FastMath.min(pair[1], pair[0]),
> > -                                Precision.EPSILON);
> > -            Assert.assertEquals("max(" + pair[0] + ", " + pair[1] +
> > ")",
> > -                                Math.max(pair[0], pair[1]),
> > -                                FastMath.max(pair[0], pair[1]),
> > -                                Precision.EPSILON);
> > -            Assert.assertEquals("max(" + pair[1] + ", " + pair[0] +
> > ")",
> > -                                Math.max(pair[1], pair[0]),
> > -                                FastMath.max(pair[1], pair[0]),
> > -                                Precision.EPSILON);
> > +            assertEquals("min(" + pair[0] + ", " + pair[1] + ")",
> > +                    Math.min(pair[0], pair[1]),
> > +                    FastMath.min(pair[0], pair[1]),
> > +                    Precision.EPSILON);
> > +            assertEquals("min(" + pair[1] + ", " + pair[0] + ")",
> > +                    Math.min(pair[1], pair[0]),
> > +                    FastMath.min(pair[1], pair[0]),
> > +                    Precision.EPSILON);
> > +            assertEquals("max(" + pair[0] + ", " + pair[1] + ")",
> > +                    Math.max(pair[0], pair[1]),
> > +                    FastMath.max(pair[0], pair[1]),
> > +                    Precision.EPSILON);
> > +            assertEquals("max(" + pair[1] + ", " + pair[0] + ")",
> > +                    Math.max(pair[1], pair[0]),
> > +                    FastMath.max(pair[1], pair[0]),
> > +                    Precision.EPSILON);
> >          }
> >      }
> >
> >      @Test
> >      public void testConstants() {
> > -        Assert.assertEquals(Math.PI, FastMath.PI, 1.0e-20);
> > -        Assert.assertEquals(Math.E, FastMath.E, 1.0e-20);
> > +        assertEquals(Math.PI, FastMath.PI, 1.0e-20);
> > +        assertEquals(Math.E, FastMath.E, 1.0e-20);
> >      }
> >
> >
> > <TRUNCATED>
> 
> 
> ---------------------------------------------------------------------
> 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


Mime
View raw message