[jira] Commented: (MATH-387) Duplicate code

["Luc Maisonobe (JIRA)" <ji...@apache.org>]

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

Luc Maisonobe commented on MATH-387:
------------------------------------

The two classes are simple implementations of the RealConvergenceChecker and VectorialConvergenceChecker. The method signature is therefore imposed by the interface.
You are right that these implementation do not use the valeu and use only the point coordinates (hence the names XxxPointChecker). However, there are other implementations of the same interfaces that do use the value (see SimpleScalarValueChecker and SimpleVectorialValueChecker). Also users may provide their own implementations of these interfaces that could use both the point and the value if they need to.

Your patch allow null points, but in fact these points are built by the optimizers and are typically never null, so I'm not sure this is useful (but I may miss something). If you consider allowing null points is useful, you should probably apply also a similar patch to the VectorialPointValuePair class.

> 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.