commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gilles (JIRA)" <j...@apache.org>
Subject [jira] Commented: (MATH-387) Duplicate code
Date Sat, 17 Jul 2010 17:24:50 GMT

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

Gilles commented on MATH-387:
-----------------------------

My observation was not that the "XxxPointChecker" use the point and and not the value (and
vice-versa for the "XxxValueChecker") but that the code is identical, which points to potential
improvement. However the changes might be too far reaching...

For algorithms that only use the value to check the convergence, I think that implementation
will be clearer if "null" is explicitely used in the constructor of the "RealPointValuePair"
passed to "SimpleScalarValueChecker". With that checker, it is also inefficient that for the
constructor to clone the "point" since it won't be used anyways.

If it's OK, I'll also change "VectorialPointValuePair" to allow "null".

> Duplicate code
> --------------
>
>                 Key: MATH-387
>                 URL: https://issues.apache.org/jira/browse/MATH-387
>             Project: Commons Math
>          Issue Type: Improvement
>    Affects Versions: 2.1
>            Reporter: Gilles
>            Priority: Minor
>             Fix For: 2.2
>
>         Attachments: RealPointValuePair.java.diff
>
>
> In package optimization:
> {code:title=SimpleRealPointChecker.java|borderStyle=solid}
> public boolean converged(final int iteration, final RealPointValuePair previous, final
RealPointValuePair current) {
>     final double[] p        = previous.getPoint();
>     final double[] c        = current.getPoint();
>     for (int i = 0; i < p.length; ++i) {
>         final double difference = Math.abs(p[i] - c[i]);
>         final double size       = Math.max(Math.abs(p[i]), Math.abs(c[i]));
>         if ((difference > (size * relativeThreshold)) && (difference >
absoluteThreshold)) {
>             return false;
>         }
>     }
>     return true;
> }
> {code}
> {code:title=SimpleVectorialPointChecker.java|borderStyle=solid}
> public boolean converged(final int iteration, final VectorialPointValuePair previous,
final VectorialPointValuePair current) {
>     final double[] p = previous.getPointRef();
>     final double[] c = current.getPointRef();
>     for (int i = 0; i < p.length; ++i) {
>         final double pi         = p[i];
>         final double ci         = c[i];
>         final double difference = Math.abs(pi - ci);
>         final double size       = Math.max(Math.abs(pi), Math.abs(ci));
>         if ((difference > (size * relativeThreshold)) &&
>             (difference > absoluteThreshold)) {
>             return false;
>         }
>     }
>     return true;
> }
> {code}
> Do they do the same thing or am I missing something?
> Also in
> {code:title=SimpleScalarValueChecker.java|borderStyle=solid}
> public boolean converged(final int iteration, final RealPointValuePair previous, final
RealPointValuePair current) {
>     final double p          = previous.getValue();
>     final double c          = current.getValue();
>     final double difference = Math.abs(p - c);
>     final double size       = Math.max(Math.abs(p), Math.abs(c));
>     return (difference <= (size * relativeThreshold)) || (difference <= absoluteThreshold);
> }
> {code}
> it seems overkill that one must create two {{RealPointValuePair}} objects when one just
wants to compare two {{double}}. Shouldn't this class contain a method like
> {code}
> public boolean converged(int iteration, double previous, double current) {
>     final double difference = Math.abs(previous - current);
>     final double size       = Math.max(Math.abs(previous), Math.abs(current));
>     return (difference <= (size * relativeThreshold)) || (difference <= absoluteThreshold);
> {code}
> ?
> Also none of these methods seem to need an {{iteration}} parameter.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message