[
https://issues.apache.org/jira/browse/MATH-418?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14007149#comment-14007149
]
Gilles commented on MATH-418:
-----------------------------
There are still things to fix.
* Usage of CM exceptions:
{code}
throw new NullArgumentException(LocalizedFormats.NULL_NOT_ALLOWED, "estimator")
{code}
must rather be
{code}
throw new NullArgumentException();
{code}
Indeed the message is the default one, and it takes no argument (hence the
string "estimator" is useless).
* You use "package" visibility in places where it must be "private" (e.g.
"postConstruct").
* I don't get why "Marker", "Markers", "PSquareInterpolatorEvaluator", ... are
"protected". It is not helpful to indicate that it is for extensibility (as
this is obvious); rather you should indicate what customization may make sense.
Until an actual need arises (TBD on the ML), all the implementation details
should be inside a "black-box" (to allow modifications without breaking
compatibility).
* IMHO, formatting should be taken care of in a separate class. The "toString"
method should not do fancy formatting. (But this issue should probably be
raised on the ML.)
* No comment in all uppercase; please. This akin to shouting. ;)
* What is the purpose of "toMap"?
* Keyword "final" is missing in several places (see e.g. method
"estimator(PSquareEstimator)", at line 1035).
* Is it really necessary to override "equals" (see e.g. at line 1066)? Since
you make strict comparisons of double values, it is unlikely that intances are
ever going to be equal...
* In Javadoc comment of public methods, do not use a @link to a "private" field
or method.
* Missing comment at lines 669 and 1368. Yes, everything must be documented. ;)
* Citing the paper more than once is redundant. The "href" link should appear
in the top class's Javadoc; then it is sufficient (and more readable) to use a
short-hand like "P-square".
Do you run the "site" target of maven and make sure that the reports are clean?
(Among other things, this runs "CheckStyle" and "FindBugs".)
A general remark: when introducing a new functionality, you should first focus
on the correctness of the implementation's main purpose, leaving out everything
that is not strictly necessary (cf. formatting, provision for extensibility,
...). The longer the code, the longer it will take to review... Whereas once a
solid base is committed, anyone can easily polish it, adding bells and whistles.
> add a storeless version of Percentile
> -------------------------------------
>
> Key: MATH-418
> URL: https://issues.apache.org/jira/browse/MATH-418
> Project: Commons Math
> Issue Type: New Feature
> Affects Versions: 2.1
> Reporter: Luc Maisonobe
> Fix For: 4.0
>
> Attachments: 418-psquare-patch, psquare-patch
>
>
> The Percentile class can handle only in-memory data.
> It would be interesting to use an on-line algorithm to estimate quantiles as
> a storeless statistic.
> An example of such an algorithm is the exponentially weighted stochastic
> approximation described in a 2000 paper by Fei Chen , Diane Lambert and
> José C. Pinheiro "Incremental Quantile Estimation for Massive Tracking" which
> can be retrieved from CiteSeerX at
> [http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.105.1580].
--
This message was sent by Atlassian JIRA
(v6.2#6252)
