[jira] [Commented] (MATH-1404) Test failure which occur with LC_ALL=en_US.UTF8, but not with LC_ALL=de_DE.UTF-8
[
https://issues.apache.org/jira/browse/MATH-1404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16508063#comment-16508063
]
Karl Richter commented on MATH-1404:
I can't check because of
{code}
[ERROR] COMPILATION ERROR :
[INFO] -
[ERROR]
/mnt/data/sources/commons-math/src/main/java/org/apache/commons/math4/analysis/solvers/LaguerreSolver.java:[20,42]
cannot find symbol
symbol: class ComplexUtils
location: package org.apache.commons.numbers.complex
[ERROR]
/mnt/data/sources/commons-math/src/main/java/org/apache/commons/math4/analysis/solvers/LaguerreSolver.java:[150,29]
cannot find symbol
symbol: variable ComplexUtils
location: class org.apache.commons.math4.analysis.solvers.LaguerreSolver
[ERROR]
/mnt/data/sources/commons-math/src/main/java/org/apache/commons/math4/analysis/solvers/LaguerreSolver.java:[196,39]
cannot find symbol
symbol: variable ComplexUtils
location: class org.apache.commons.math4.analysis.solvers.LaguerreSolver
[ERROR]
/mnt/data/sources/commons-math/src/main/java/org/apache/commons/math4/analysis/solvers/LaguerreSolver.java:[226,36]
cannot find symbol
symbol: variable ComplexUtils
location: class org.apache.commons.math4.analysis.solvers.LaguerreSolver
[INFO] 4 errors
{code}
(probably worth a separate report).
> Test failure which occur with LC_ALL=en_US.UTF8, but not with
> LC_ALL=de_DE.UTF-8
>
>
> Key: MATH-1404
> URL: https://issues.apache.org/jira/browse/MATH-1404
> Project: Commons Math
> Issue Type: Bug
>Reporter: Karl Richter
>Priority: Major
>
> Specifying `LC_ALL=en_US.UTF-8` causes test failures like
> `RandomUtilsDataGeneratorJDKSecureRandomTest>RandomUtilsDataGeneratorAbstractTest.testNextLongNegativeRange:99->RandomUtilsDataGeneratorAbstractTest.checkNextLongUniform:130
> Chisquare test failed p-value = 0.0082266579945659 chisquare statistic =
> 25.3040006.` which don't seem to occur with `LC_ALL=de_DE.UTF-8`, see
> https://travis-ci.org/krichter722/commons-math/jobs/205896548 for details
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (MATH-1404) Test failure which occur with LC_ALL=en_US.UTF8, but not with LC_ALL=de_DE.UTF-8
[ https://issues.apache.org/jira/browse/MATH-1404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16509251#comment-16509251 ] Karl Richter commented on MATH-1404: The issue no longer occurs (tested with the compilation error fix). > Test failure which occur with LC_ALL=en_US.UTF8, but not with > LC_ALL=de_DE.UTF-8 > > > Key: MATH-1404 > URL: https://issues.apache.org/jira/browse/MATH-1404 > Project: Commons Math > Issue Type: Bug >Reporter: Karl Richter >Priority: Major > > Specifying `LC_ALL=en_US.UTF-8` causes test failures like > `RandomUtilsDataGeneratorJDKSecureRandomTest>RandomUtilsDataGeneratorAbstractTest.testNextLongNegativeRange:99->RandomUtilsDataGeneratorAbstractTest.checkNextLongUniform:130 > Chisquare test failed p-value = 0.0082266579945659 chisquare statistic = > 25.3040006.` which don't seem to occur with `LC_ALL=de_DE.UTF-8`, see > https://travis-ci.org/krichter722/commons-math/jobs/205896548 for details -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (LANG-1382) Provide method to generate random numbers without range restriction for all numeric types
[ https://issues.apache.org/jira/browse/LANG-1382?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16584965#comment-16584965 ] Karl Richter commented on LANG-1382: bq. It is provided by DiscreteUniformSampler. Does the job. > Provide method to generate random numbers without range restriction for all > numeric types > - > > Key: LANG-1382 > URL: https://issues.apache.org/jira/browse/LANG-1382 > Project: Commons Lang > Issue Type: New Feature >Reporter: Karl Richter >Priority: Major > > `RandomUtils` only allows to generate random numbers of different numeric > types (`byte`, `short`, `double`, etc.) within the range 0 (inclusive) and > the maximum value of the respective type. It'd be nice if there were methods > to create random numbers for the range between the minimal and the maximal > value for all numeric types. > I'm aware of workarounds like > https://stackoverflow.com/questions/27976857/how-to-get-random-number-with-negative-number-in-range, > but they're not tested and require quite some thinking which can be avoided > (which is afaik one of the many motivations for the Apache commons libraries). > If such methods already exist they should be definitely linked in the Javadoc > of `RandomUtils`. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Resolved] (LANG-1382) Provide method to generate random numbers without range restriction for all numeric types
[ https://issues.apache.org/jira/browse/LANG-1382?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter resolved LANG-1382. Resolution: Not A Problem > Provide method to generate random numbers without range restriction for all > numeric types > - > > Key: LANG-1382 > URL: https://issues.apache.org/jira/browse/LANG-1382 > Project: Commons Lang > Issue Type: New Feature >Reporter: Karl Richter >Priority: Major > > `RandomUtils` only allows to generate random numbers of different numeric > types (`byte`, `short`, `double`, etc.) within the range 0 (inclusive) and > the maximum value of the respective type. It'd be nice if there were methods > to create random numbers for the range between the minimal and the maximal > value for all numeric types. > I'm aware of workarounds like > https://stackoverflow.com/questions/27976857/how-to-get-random-number-with-negative-number-in-range, > but they're not tested and require quite some thinking which can be avoided > (which is afaik one of the many motivations for the Apache commons libraries). > If such methods already exist they should be definitely linked in the Javadoc > of `RandomUtils`. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Created] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
Karl Richter created MATH-1426: -- Summary: 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 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. I can't attach files here, so let me know how to get this merged or why you strictly oppose it. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Attachment: 0001-fixed-javadoc-of-constructors-in-DescriptiveStatisti.patch 0002-added-constructor-with-Double-argument-to-Descriptiv.patch `git format-patch HEAD^^` > 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. I can't attach files here, so let me know how to get this > merged or why you strictly oppose it. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Description: 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. was: 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. I can't attach files here, so let me know how to get this merged or why you strictly oppose it. > 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)
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Attachment: 0002-added-constructor-with-Double-argument-to-Descriptiv.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)
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Attachment: (was: 0002-added-constructor-with-Double-argument-to-Descriptiv.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 > > > 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)
[jira] [Commented] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16114221#comment-16114221 ] Karl Richter commented on MATH-1426: > A unit test would be welcome. Done. Travis CI failed because of a crash of the JVM, please restart it. > 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)
[jira] [Comment Edited] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16114221#comment-16114221 ] Karl Richter edited comment on MATH-1426 at 8/4/17 10:29 AM: - > A unit test would be welcome. Done. Coverage increased. Travis CI failed because of a crash of the JVM, please restart it. was (Author: krichter): > A unit test would be welcome. Done. Travis CI failed because of a crash of the JVM, please restart it. > 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)
[jira] [Created] (EXEC-104) Test failure "Exec34Test.testExec34_2:88 Watchdog should have killed the process"
Karl Richter created EXEC-104: - Summary: Test failure "Exec34Test.testExec34_2:88 Watchdog should have killed the process" Key: EXEC-104 URL: https://issues.apache.org/jira/browse/EXEC-104 Project: Commons Exec Issue Type: Bug Reporter: Karl Richter [Travis CI reveals the following test failures](https://travis-ci.org/apache/commons-exec/jobs/237502021): ``` Exec34Test.testExec34_2:88 Watchdog should have killed the process Exec41Test.testExec41WithStreams:96 The process was killed by the watchdog Exec41Test.testExec41WithoutStreams:143 The process was killed by the watchdog ``` If it's a Travis CI issue, the script should be fixed to avoid or suppress these issues. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[
https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16114457#comment-16114457
]
Karl Richter commented on MATH-1426:
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).
> For random generation, please use the classes from Commons RNG (cf. examples
> in other test classes).
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}
checkstyle
check
validate
{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)
[jira] [Comment Edited] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[
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:46 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?
> 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}
checkstyle
check
validate
{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).
> For random generation, please use the classes from Commons RNG (cf. examples
> in other test classes).
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 1
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Attachment: (was: 0002-added-constructor-with-Double-argument-to-Descriptiv.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)
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Attachment: 0002-added-constructor-with-Double-argument-to-Descriptiv.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)
[jira] [Comment Edited] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[
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}
checkstyle
check
validate
{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 commente
[jira] [Commented] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16114581#comment-16114581 ] Karl Richter commented on MATH-1426: bq. So, in this issue's case, using a randomly populated array is just a convenience: the code could be checked with a "manually" chosen set of values; hence, we can choose any seed (and keep that one forever). Interesting, thanks for the detailed explanation. I imitated the code of other tests as suggested. bq. For "Commons Math" main code (in the "main" directory), only the API part would become a dependency. I also think that "slf4j-api" is a safe choice (but this must be decided on the ML). One discussion is http://markmail.org/message/pf46s7775bt37swk for reference. That's tricky because of the wide range of interests. SLF4J is the most often declared dependency in github.com projectshttps://en.wikipedia.org/wiki/SLF4J. bq. I'm no maven expert; please raise this issue on the "dev" ML (use different posts for different subjects). Done. The title is `[MATH] Enforce run of checkstyle-maven-plugin in validate instead of site phase` bq. I didn't figure out what you mean by wrong tabulation bq. For example, I'd think that in most of the code, the alignment would be (to be viewed with a fixed-width font...): I took over your example, the rest will be covered by checkstyle enforcement. > 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)
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Attachment: (was: 0002-added-constructor-with-Double-argument-to-Descriptiv.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)
[jira] [Updated] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1426: --- Attachment: 0002-added-constructor-with-Double-argument-to-Descriptiv.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)
[jira] [Comment Edited] (MATH-1426) Add constructor with Double[] argument to DescriptiveStatistics
[ https://issues.apache.org/jira/browse/MATH-1426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16114581#comment-16114581 ] Karl Richter edited comment on MATH-1426 at 8/4/17 4:25 PM: bq. So, in this issue's case, using a randomly populated array is just a convenience: the code could be checked with a "manually" chosen set of values; hence, we can choose any seed (and keep that one forever). Interesting, thanks for the detailed explanation. I imitated the code of other tests as suggested. bq. For "Commons Math" main code (in the "main" directory), only the API part would become a dependency. bq. I also think that "slf4j-api" is a safe choice (but this must be decided on the ML). One discussion is http://markmail.org/message/pf46s7775bt37swk for reference. That's tricky because of the wide range of interests. SLF4J is the most often declared dependency in github.com projectshttps://en.wikipedia.org/wiki/SLF4J. bq. I'm no maven expert; please raise this issue on the "dev" ML (use different posts for different subjects). Done. The title is `[MATH] Enforce run of checkstyle-maven-plugin in validate instead of site phase` bq. I didn't figure out what you mean by wrong tabulation bq. For example, I'd think that in most of the code, the alignment would be (to be viewed with a fixed-width font...): I took over your example, the rest will be covered by checkstyle enforcement. was (Author: krichter): bq. So, in this issue's case, using a randomly populated array is just a convenience: the code could be checked with a "manually" chosen set of values; hence, we can choose any seed (and keep that one forever). Interesting, thanks for the detailed explanation. I imitated the code of other tests as suggested. bq. For "Commons Math" main code (in the "main" directory), only the API part would become a dependency. I also think that "slf4j-api" is a safe choice (but this must be decided on the ML). One discussion is http://markmail.org/message/pf46s7775bt37swk for reference. That's tricky because of the wide range of interests. SLF4J is the most often declared dependency in github.com projectshttps://en.wikipedia.org/wiki/SLF4J. bq. I'm no maven expert; please raise this issue on the "dev" ML (use different posts for different subjects). Done. The title is `[MATH] Enforce run of checkstyle-maven-plugin in validate instead of site phase` bq. I didn't figure out what you mean by wrong tabulation bq. For example, I'd think that in most of the code, the alignment would be (to be viewed with a fixed-width font...): I took over your example, the rest will be covered by checkstyle enforcement. > 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)
[jira] [Created] (LANG-1382) Provide method to generate random numbers without range restriction for all numeric types
Karl Richter created LANG-1382: -- Summary: Provide method to generate random numbers without range restriction for all numeric types Key: LANG-1382 URL: https://issues.apache.org/jira/browse/LANG-1382 Project: Commons Lang Issue Type: New Feature Reporter: Karl Richter `RandomUtils` only allows to generate random numbers of different numeric types (`byte`, `short`, `double`, etc.) within the range 0 (inclusive) and the maximum value of the respective type. It'd be nice if there were methods to create random numbers for the range between the minimal and the maximal value for all numeric types. I'm aware of workarounds like https://stackoverflow.com/questions/27976857/how-to-get-random-number-with-negative-number-in-range, but they're not tested and require quite some thinking which can be avoided (which is afaik one of the many motivations for the Apache commons libraries). If such methods already exist they should be definitely linked in the Javadoc of `RandomUtils`. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (LANG-1382) Provide method to generate random numbers without range restriction for all numeric types
[ https://issues.apache.org/jira/browse/LANG-1382?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16383788#comment-16383788 ] Karl Richter commented on LANG-1382: > Did you know "Commons RNG"? Now, that you mention it... `JDKRandom` of Commons RNG provides negative and positive values. Imo it still makes a lot of sense to add a link to the Commons Lang Javadoc. > Provide method to generate random numbers without range restriction for all > numeric types > - > > Key: LANG-1382 > URL: https://issues.apache.org/jira/browse/LANG-1382 > Project: Commons Lang > Issue Type: New Feature >Reporter: Karl Richter >Priority: Major > > `RandomUtils` only allows to generate random numbers of different numeric > types (`byte`, `short`, `double`, etc.) within the range 0 (inclusive) and > the maximum value of the respective type. It'd be nice if there were methods > to create random numbers for the range between the minimal and the maximal > value for all numeric types. > I'm aware of workarounds like > https://stackoverflow.com/questions/27976857/how-to-get-random-number-with-negative-number-in-range, > but they're not tested and require quite some thinking which can be avoided > (which is afaik one of the many motivations for the Apache commons libraries). > If such methods already exist they should be definitely linked in the Javadoc > of `RandomUtils`. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Comment Edited] (LANG-1382) Provide method to generate random numbers without range restriction for all numeric types
[ https://issues.apache.org/jira/browse/LANG-1382?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16383788#comment-16383788 ] Karl Richter edited comment on LANG-1382 at 3/2/18 4:52 PM: > Did you know "Commons RNG"? Now, that you mention it... `JDKRandom` of Commons RNG provides negative and positive values. However, there's still functionality missing to create values in between a specified range. was (Author: krichter): > Did you know "Commons RNG"? Now, that you mention it... `JDKRandom` of Commons RNG provides negative and positive values. Imo it still makes a lot of sense to add a link to the Commons Lang Javadoc. > Provide method to generate random numbers without range restriction for all > numeric types > - > > Key: LANG-1382 > URL: https://issues.apache.org/jira/browse/LANG-1382 > Project: Commons Lang > Issue Type: New Feature >Reporter: Karl Richter >Priority: Major > > `RandomUtils` only allows to generate random numbers of different numeric > types (`byte`, `short`, `double`, etc.) within the range 0 (inclusive) and > the maximum value of the respective type. It'd be nice if there were methods > to create random numbers for the range between the minimal and the maximal > value for all numeric types. > I'm aware of workarounds like > https://stackoverflow.com/questions/27976857/how-to-get-random-number-with-negative-number-in-range, > but they're not tested and require quite some thinking which can be avoided > (which is afaik one of the many motivations for the Apache commons libraries). > If such methods already exist they should be definitely linked in the Javadoc > of `RandomUtils`. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Created] (MATH-1399) test failure due to "CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but was:<0.7153950105848449>"
Karl Richter created MATH-1399: -- Summary: test failure due to "CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but was:<0.7153950105848449>" Key: MATH-1399 URL: https://issues.apache.org/jira/browse/MATH-1399 Project: Commons Math Issue Type: Bug Affects Versions: 3.4.1 Environment: travis-ci.org Reporter: Karl Richter `mvn test` fails due to `CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but was:<0.7153950105848449>`, see attached log for details experienced with MATH_3_4_1_RC1-718-g75b98fa -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Updated] (MATH-1399) test failure due to "CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but was:<0.7153950105848449>"
[ https://issues.apache.org/jira/browse/MATH-1399?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1399: --- Description: `mvn test` randomly fails due to `CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but was:<0.7153950105848449>`, see attached log for details experienced with MATH_3_4_1_RC1-718-g75b98fa was: `mvn test` fails due to `CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but was:<0.7153950105848449>`, see attached log for details experienced with MATH_3_4_1_RC1-718-g75b98fa > test failure due to "CMAESOptimizerTest.testMaximize:155->doTest:510 > expected:<1.0> but was:<0.7153950105848449>" > - > > Key: MATH-1399 > URL: https://issues.apache.org/jira/browse/MATH-1399 > Project: Commons Math > Issue Type: Bug >Affects Versions: 3.4.1 > Environment: travis-ci.org >Reporter: Karl Richter > Attachments: travis-ci.org.log.tar.xz > > > `mvn test` randomly fails due to > `CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but > was:<0.7153950105848449>`, see attached log for details > experienced with MATH_3_4_1_RC1-718-g75b98fa -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Updated] (MATH-1399) test failure due to "CMAESOptimizerTest.testMaximize:155->doTest:510 expected:<1.0> but was:<0.7153950105848449>"
[ https://issues.apache.org/jira/browse/MATH-1399?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Karl Richter updated MATH-1399: --- Attachment: travis-ci.org.log.tar.xz > test failure due to "CMAESOptimizerTest.testMaximize:155->doTest:510 > expected:<1.0> but was:<0.7153950105848449>" > - > > Key: MATH-1399 > URL: https://issues.apache.org/jira/browse/MATH-1399 > Project: Commons Math > Issue Type: Bug >Affects Versions: 3.4.1 > Environment: travis-ci.org >Reporter: Karl Richter > Attachments: travis-ci.org.log.tar.xz > > > `mvn test` fails due to `CMAESOptimizerTest.testMaximize:155->doTest:510 > expected:<1.0> but was:<0.7153950105848449>`, see attached log for details > experienced with MATH_3_4_1_RC1-718-g75b98fa -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Created] (MATH-1404) Test failure which occur with LC_ALL=en_US.UTF8, but not with LC_ALL=de_DE.UTF-8
Karl Richter created MATH-1404: -- Summary: Test failure which occur with LC_ALL=en_US.UTF8, but not with LC_ALL=de_DE.UTF-8 Key: MATH-1404 URL: https://issues.apache.org/jira/browse/MATH-1404 Project: Commons Math Issue Type: Bug Reporter: Karl Richter Specifying `LC_ALL=en_US.UTF-8` causes test failures like `RandomUtilsDataGeneratorJDKSecureRandomTest>RandomUtilsDataGeneratorAbstractTest.testNextLongNegativeRange:99->RandomUtilsDataGeneratorAbstractTest.checkNextLongUniform:130 Chisquare test failed p-value = 0.0082266579945659 chisquare statistic = 25.3040006.` which don't seem to occur with `LC_ALL=de_DE.UTF-8`, see https://travis-ci.org/krichter722/commons-math/jobs/205896548 for details -- This message was sent by Atlassian JIRA (v6.3.15#6346)
