Re: [5/7] [math] Fix "FastMath#round(..)" to comply to changed contract of "Math#round()" in Java 8

Sun, 07 Aug 2016 15:39:04 -0700 

On Sat, 6 Aug 2016 11:46:01 -0700, Dennis E. Hamilton wrote:

-----Original Message-----
From: Gilles [mailto:gil...@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).


In CM, the usage was to not use "import static".


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?


Excerpts from "CONTRIBUTING.md":
+ Create a topic branch from where you want to base your work (this is usually the develop/trunk branch). 
+ Make commits of logical units.
+ Respect the original code style:
+ Create minimal diffs - disable on save actions like reformat source code or organize imports. If you feel the source code should be reformatted create a separate PR for this change. + Make sure your commit messages are in the proper format. Your commit message should contain 
  the key of the JIRA issue.
+ For example, a commit message might look like `MATH-852: Adding documentation for development` 


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.)


If there is a JIRA report, the log must indeed make it straightforward
to find it.

Gilles


 - Dennis




On Fri, 05 Aug 2016 15:06:53 -0000, ebo...@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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Reply via email to