You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Gilles (JIRA)" <ji...@apache.org> on 2010/07/16 16:56:49 UTC
[jira] Created: (MATH-387) Duplicate code
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
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.
[jira] Resolved: (MATH-387) Duplicate code
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-387?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Gilles resolved MATH-387.
-------------------------
Resolution: Fixed
Changed in revision 965509.
Although, because of this:
{quote}
The optimizer that will construct the point does not know beforehand that the convergence checker selected by the user will use or not the point [...]
{quote}
I now wonder whether it will really be useful to allow "null"!
Another question is whether one can meaningfully separate the optimization algorithm from the convergence checker: Can one always choose a checker independently of the optimization algorithm?
> 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.
[jira] Commented: (MATH-387) Duplicate code
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Commented: (MATH-387) Duplicate code
Posted by "Luc Maisonobe (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-387?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12889530#action_12889530 ]
Luc Maisonobe commented on MATH-387:
------------------------------------
The optimizer that will construct the point does not know beforehand that the convergence checker selected by the user will use or not the point, so I don't think we can optimized away the cloning.
You can go ahead with allowing null everywhere, it is an improvement for the class that may be useful for some specific optimizers later on.
> 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.
[jira] Updated: (MATH-387) Duplicate code
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-387?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Gilles updated MATH-387:
------------------------
Attachment: RealPointValuePair.java.diff
Forget about the initial description; fixing it will probably entail incompatible changes.
Nevertheless I'd like to modify "RealPointValuePair.java" (see attached diff) so that it can be made fairly obvious (in an algorithm's code) that the convergence checker only uses the value of the objective function.
Objections?
> 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.
[jira] Commented: (MATH-387) Duplicate code
Posted by "Luc Maisonobe (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-387?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12889947#action_12889947 ]
Luc Maisonobe commented on MATH-387:
------------------------------------
>From my experience, convergence is more related to the problem to be solved than to the optimizer used.
> 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.
[jira] Commented: (MATH-387) Duplicate code
Posted by "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.