commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Karl Richter (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
Date Fri, 04 Aug 2017 14:48:00 GMT

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

Mime
View raw message