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