You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Arne Plöse (JIRA)" <ji...@apache.org> on 2011/07/20 11:46:58 UTC
[jira] [Created] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Use implementation of JAMA for ArrayRealVector operators
--------------------------------------------------------
Key: MATH-623
URL: https://issues.apache.org/jira/browse/MATH-623
Project: Commons Math
Issue Type: Improvement
Affects Versions: 3.0
Reporter: Arne Plöse
Priority: Minor
For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
I will provide a patch with some more enhancements/cleanups for this.
in the test look for XXX:
the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068453#comment-13068453 ]
Gilles commented on MATH-623:
-----------------------------
If nobody objects, and since one less {{clone}} is good, I'll change the {{add(double[] v)}} method as per Arne's suggestion.
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068344#comment-13068344 ]
Gilles commented on MATH-623:
-----------------------------
I confirm that the efficiency improvement for {{add}} tends to increase with the size of the arrays.
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068651#comment-13068651 ]
Gilles commented on MATH-623:
-----------------------------
You are welcome to provide patches, but one issue at a time. :)
I'll update the code for {{add}}. Any further improvement to {{ArrayRealVector}}, you can post here.
For touching other classes, please open one or several new issues.
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Arne Plöse (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068275#comment-13068275 ]
Arne Plöse commented on MATH-623:
---------------------------------
Sorry, I just point out where I think things should go ...
So at first let look at ArrayRealVector.add and then to (OpenMap|Array)(Real|Field)(Vector|Matrix).(add|subtract|...).
After that wee will see...
{code}
@Override
public RealVector add(RealVector v) {
if (v instanceof ArrayRealVector) {
return add(((ArrayRealVector)v).data);
} else if (v instanceof SparseRealVector){
checkVectorDimensions(v);
double[] out = data.clone();
Iterator<Entry> it = ((SparseRealVector)v).sparseIterator();
Entry e;
while (it.hasNext() && (e = it.next()) != null) {
out[e.getIndex()] += e.getValue();
}
return new ArrayRealVector(out, false);
} else {
final int dim = v.getDimension();
checkVectorDimensions(dim);
ArrayRealVector result = new ArrayRealVector(dim);
double[] resultData = result.data;
for (int i = 0; i < dim; i++) {
resultData[i] = data[i] + v.getEntry(i);
}
return result;
}
}
RealVector addOld(double[] v) {
final int dim = v.length;
checkVectorDimensions(dim);
double[] out = data.clone();
for (int i = 0; i < dim; i++) {
out[i] += v[i];
}
return new ArrayRealVector(out, false);
}
@Override
public RealVector add(double[] v) {
final int dim = v.length;
checkVectorDimensions(dim);
ArrayRealVector result = new ArrayRealVector(dim);
double[] resultData = result.data;
for (int i = 0; i < dim; i++) {
resultData[i] = data[i] + v[i];
}
return result;
}
{code}
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13069281#comment-13069281 ]
Gilles commented on MATH-623:
-----------------------------
Patch applied in revision 1149405.
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff, ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Arne Plöse (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Arne Plöse updated MATH-623:
----------------------------
Attachment: ArrayRealVector.diff
Here ist the diff with added testcase
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Arne Plöse (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Arne Plöse updated MATH-623:
----------------------------
Attachment: ArrayRealVector.diff
here the changes for any *(double[] v) methods.
Why not change
{code}
public RealVector add(RealVector v) {...}
{code}
to this:
{code}
@Override
public RealVector add(RealVector v) {
if (v instanceof ArrayRealVector) {
return add(((ArrayRealVector) v).data);
} else {
checkVectorDimensions(v);
double[] out = data.clone();
Iterator<Entry> it = v.sparseIterator();
Entry e;
while (it.hasNext() && (e = it.next()) != null) {
out[e.getIndex()] += e.getValue();
}
return new ArrayRealVector(out, false);
}
}
{code}
and drop
{code}
public ArrayRealVector add(ArrayRealVector v) {...}
{code}
it only exposes the implementation of RealVector and I think the compiler knows seldom of ArrayRealvector as argument...
By the way there is no need for /** {@inheritDoc} */ on methods with @Override annotation, maybe remove this commnet in this case?
Stupid question dou you want a new issue for all of these?
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff, ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068271#comment-13068271 ]
Gilles commented on MATH-623:
-----------------------------
Hello Arne.
The _strongly_ recommended work method for modifying the code is to separate issues as much as possible. So, in this case, you propose an enhancement to the {{add}} method implementation in class {{ArrayRealVector}} but the diff contains many other modifications that might possibly be introduced later. If the changes are not interdependent, it is much easier to examine the merits of several changes one at a time.
Especially, the interface change of {{RealVector}} and {{SparseRealVector}} may be justified for their own sake, and do not seem to be directly dependent on the performance issue.
Thanks in advance for your patience...
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068671#comment-13068671 ]
Gilles commented on MATH-623:
-----------------------------
New {{add}} implementation committed in revision 1148952.
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Arne Plöse (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068499#comment-13068499 ]
Arne Plöse commented on MATH-623:
---------------------------------
If you do that, look out for other occurences like subtract(double[])..., an then for ArrayRealMatrix and so on.
Happy coding ;-)
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Gilles resolved MATH-623.
-------------------------
Resolution: Fixed
Fix Version/s: 3.0
Main issue fixed.
New tickets have been opened for the others.
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Fix For: 3.0
>
> Attachments: ArrayRealVector.diff, ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-623) Use implementation of JAMA for
ArrayRealVector operators
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13069287#comment-13069287 ]
Gilles commented on MATH-623:
-----------------------------
{quote}
Stupid question dou you want a new issue for all of these?
{quote}
Independent issues should be handled separately. The more so if they are totally unrelated like your suggestions about "{@inheritDoc}" and changing/removing some methods.
> Use implementation of JAMA for ArrayRealVector operators
> --------------------------------------------------------
>
> Key: MATH-623
> URL: https://issues.apache.org/jira/browse/MATH-623
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Arne Plöse
> Priority: Minor
> Attachments: ArrayRealVector.diff, ArrayRealVector.diff
>
>
> For instance the add(double[] v) fist clones the array, and then adds all entries of this.data to the result.
> JAMA uses the following approach create a empty result[] the assign the sum of each entry to the result. this is approximately 10 -20 % faster.
> I will provide a patch with some more enhancements/cleanups for this.
> in the test look for XXX:
> the first number is the jama time the second the current algorithm.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira