Mailing-List: contact issues-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: issues@commons.apache.org
Date: Fri, 4 Aug 2017 14:48:00 +0000 (UTC)
From: "Karl Richter (JIRA)" <jira@apache.org>
To: issues@commons.apache.org
Message-ID: <JIRA.13090587.1501179217000.94451.1501858080271@Atlassian.JIRA>
In-Reply-To: <JIRA.13090587.1501179217000@Atlassian.JIRA>
References: <JIRA.13090587.1501179217000@Atlassian.JIRA> <JIRA.13090587.1501179217939@jira-lw-us.apache.org>
Subject: [jira] [Comment Edited] (MATH-1426) Add constructor with Double[]
 argument to DescriptiveStatistics
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Fri, 04 Aug 2017 14:48:06 -0000


    [ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16114457#comment-16114457 ] 

Karl Richter edited comment on MATH-1426 at 8/4/17 2:47 PM:
------------------------------------------------------------

Thanks for your review. You have impressive coding discipline!

> Prefer several small test methods (one for each tested functionality) even if some boiler-plate code repetition do occur. Having "testInit1()", "testInit2()", ... is fine (but more meaningful names are preferred if possible).

Done.

> If using random data, use a fixed seed, unless the behaviour under test has intrinsic variability (which is not the case here).

I don't see the downside of using a different "fixed" ("fixed" for the run of the test, not "fixed" as you mean it) seed for each run of the unit tests, e.g. based on the test start time millis, and log the seed so that reproduction in case of test failure is possible since we're working with pseudo-random generators which can be re-initialized with the same seed to produce the same pseudo-random results. In case you agree, how do I initialize a Commons RNG with such a variable seed? If you don't I'll comply with your explanation.

> Make test sets small (as long as they can reasonably check the functionality) to avoid long-running "mvn test"; here I don't think that arrays of length 1048576 were needed.

Agreed. I chose 1024.

> Comment out debugging output ("System.out.println")

I'm a huge fan of configurable logging framework which on the one hand require the evaluation of one very cheap statement, but on the other minimize controversy between devs and avoid adding previously deleted code (since you can turn logging statements off in your `logback.xml` or whather is used). I wouldn't speak of "debugging" statements since it's either debugging or logging. The logging of the generator seed is necessary unless the above is wrong, I guess you'll shed some light on this issue.

Do you have interest in adding a logging framework (I suggest slf4j-api + logback-classic). There're about 100 System.out.print statements in the code, some commented out. I'd provide a patch if you want.

> Apply a uniform coding style (e.g. there must be a space around operators, and the tabulation is wrong).

I suggest you move `maven-checkstyle-plugin` from `reporting` to `build` with 
{code:java}
        <executions>
          <execution>
            <id>checkstyle</id>
            <goals>
              <goal>check</goal>
            </goals>
            <phase>validate</phase>
          </execution>
        </executions>
{code}
That reveals some hundred issues which should either be silenced or fixed (almost all of them are Javadoc issues which you might deactive for the check and put on your schedule to fix later). It minimizes communication overhead before reviewing contributions like in this situation. The issues you're describing are not revealed by checkstyle and I didn't figure out what you mean by wrong tabulation - no need to explain if you change the checkstyle, since then I can rebase the patch.


was (Author: krichter):
Thanks for your review. You have impressive coding discipline!

> Prefer several small test methods (one for each tested functionality) even if some boiler-plate code repetition do occur. Having "testInit1()", "testInit2()", ... is fine (but more meaningful names are preferred if possible).

Done.

> If using random data, use a fixed seed, unless the behaviour under test has intrinsic variability (which is not the case here).

I don't see the downside of using a different "fixed" ("fixed" for the run of the test, not "fixed" as you mean it) seed for each run of the unit tests, e.g. based on the test start time millis, and log the seed so that reproduction in case of test failure is possible since we're working with pseudo-random generators which can be re-initialized with the same seed to produce the same pseudo-random results. In case you agree, how do I initialize a Commons RNG with such a variable seed?

> Make test sets small (as long as they can reasonably check the functionality) to avoid long-running "mvn test"; here I don't think that arrays of length 1048576 were needed.

Agreed. I chose 1024.

> Comment out debugging output ("System.out.println")

I'm a huge fan of configurable logging framework which on the one hand require the evaluation of one very cheap statement, but on the other minimize controversy between devs and avoid adding previously deleted code (since you can turn logging statements off in your `logback.xml` or whather is used). I wouldn't speak of "debugging" statements since it's either debugging or logging. The logging of the generator seed is necessary unless the above is wrong, I guess you'll shed some light on this issue.

Do you have interest in adding a logging framework (I suggest slf4j-api + logback-classic). There're about 100 System.out.print statements in the code, some commented out. I'd provide a patch if you want.

> Apply a uniform coding style (e.g. there must be a space around operators, and the tabulation is wrong).

I suggest you move `maven-checkstyle-plugin` from `reporting` to `build` with 
{code:java}
        <executions>
          <execution>
            <id>checkstyle</id>
            <goals>
              <goal>check</goal>
            </goals>
            <phase>validate</phase>
          </execution>
        </executions>
{code}
That reveals some hundred issues which should either be silenced or fixed (almost all of them are Javadoc issues which you might deactive for the check and put on your schedule to fix later). It minimizes communication overhead before reviewing contributions like in this situation. The issues you're describing are not revealed by checkstyle and I didn't figure out what you mean by wrong tabulation - no need to explain if you change the checkstyle, since then I can rebase the patch.

> Add constructor with Double[] argument to DescriptiveStatistics
> ---------------------------------------------------------------
>
>                 Key: MATH-1426
>                 URL: https://issues.apache.org/jira/browse/MATH-1426
>             Project: Commons Math
>          Issue Type: Improvement
>    Affects Versions: 4.0
>            Reporter: Karl Richter
>             Fix For: 4.0
>
>         Attachments: 0001-fixed-javadoc-of-constructors-in-DescriptiveStatisti.patch, 0002-added-constructor-with-Double-argument-to-Descriptiv.patch
>
>
> It'd be nice to have a `Double[]` constructor in `DescriptiveStatistics`.
> The patch is available at https://github.com/apache/commons-math/pull/54 in form of a PR as well.


--
This message was sent by Atlassian JIRA
(v6.4.14#64029)