commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sébastien Brisard <sebastien.bris...@m4x.org>
Subject Re: svn commit: r1348721 - in /commons/proper/math/trunk/src: main/java/org/apache/commons/math3/linear/OpenMapRealVector.java test/java/org/apache/commons/math3/linear/SparseRealVectorTest.java
Date Mon, 11 Jun 2012 15:30:00 GMT
Hi Gilles,

2012/6/11 Gilles Sadowski <gilles@harfang.homelinux.org>:
> Hello.
>
>> Date: Mon Jun 11 05:52:16 2012
>> New Revision: 1348721
>>
>> URL: http://svn.apache.org/viewvc?rev=1348721&view=rev
>> Log:
>> MATH-803:
>>   - modified OpenMapRealVector.ebeMultiply() and ebeDivide() to handle
>> special cases  0d * NaN, 0d * Infinity, 0d / 0d and 0d / NaN.
>>   - added implementation of isNaN() and isInfinite() to
>> SparseRealVectorTest.SparseRealVectorTestImpl in order to allow for testing
>> of OpenMapRealVector.ebeMultiply() and ebeDivide() with mixed types.
>>
>> Modified:
>>     commons/proper/math/trunk/src/main/java/org/apache/commons/math3/linear/OpenMapRealVector.java
>>     commons/proper/math/trunk/src/test/java/org/apache/commons/math3/linear/SparseRealVectorTest.java
>>
>> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math3/linear/OpenMapRealVector.java
>> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math3/linear/OpenMapRealVector.java?rev=1348721&r1=1348720&r2=1348721&view=diff
>> ==============================================================================
>> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math3/linear/OpenMapRealVector.java
(original)
>> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math3/linear/OpenMapRealVector.java
Mon Jun 11 05:52:16 2012
>> @@ -341,10 +341,14 @@ public class OpenMapRealVector extends S
>>      public OpenMapRealVector ebeDivide(RealVector v) {
>>          checkVectorDimensions(v.getDimension());
>>          OpenMapRealVector res = new OpenMapRealVector(this);
>> -        Iterator iter = entries.iterator();
>> -        while (iter.hasNext()) {
>> -            iter.advance();
>> -            res.setEntry(iter.key(), iter.value() / v.getEntry(iter.key()));
>> +        /*
>> +         * MATH-803: it is not sufficient to loop through non zero entries of
>> +         * this only. Indeed, if this[i] = 0d and v[i] = 0d, then
>> +         * this[i] / v[i] = NaN, and not 0d.
>> +         */
>> +        final int n = getDimension();
>> +        for (int i = 0; i < n; i++) {
>> +            res.setEntry(i, this.getEntry(i) / v.getEntry(i));
>>          }
>>          return res;
>>      }
>
> I think that this renders the class potentially useless, if we assume that
> it is used when "large" vectors are needed.
> Indeed, after this operation, all the entries will be stored; thus
> cancelling the memory efficiency of the class, and probably making the
> returned value slower than an "ArrayRealVector" instance for subsequent
> operations.
>
I'm not sure that all entries would be stored (except if setEntry does
not distinguish between zero values and non-zero values).

> For every method that a "RealVector" argument, there should be a specialized
> implementation that take an "OpenMapRealVector".
>
> Also, couldn't we solve some of these problems if the value of the default
> entry was stored and mutable? E.g. if the default for "v" and "w" is zero,
> then the default for "v / w" will be "NaN".
>
>> @@ -359,6 +363,25 @@ public class OpenMapRealVector extends S
>>              iter.advance();
>>              res.setEntry(iter.key(), iter.value() * v.getEntry(iter.key()));
>>          }
>> +        /*
>> +         * MATH-803: the above loop assumes that 0d * x  = 0d for any double
x,
>> +         * which allows to consider only the non-zero entries of this. However,
>> +         * this fails if this[i] == 0d and (v[i] = NaN or v[i] = Infinity).
>> +         *
>> +         * These special cases are handled below.
>> +         */
>> +        if (v.isNaN() || v.isInfinite()) {
>> +            final int n = getDimension();
>> +            for (int i = 0; i < n; i++) {
>> +                final double y = v.getEntry(i);
>> +                if (Double.isNaN(y)) {
>> +                    res.setEntry(i, Double.NaN);
>> +                } else if (Double.isInfinite(y)) {
>> +                    final double x = this.getEntry(i);
>> +                    res.setEntry(i, x * y);
>> +                }
>> +            }
>> +        }
>
> This probably can only be a temporary solution: 3 additional loops over all
> the elements...
>
If we cache isNaN and isInfinite, this is "only" one additional loop ;).
I don't like this solution either, but the bugs I have identified are
very real, except if we accept that exceptional cases are not handled
correctly by sparse vectors. I'm personally adverse to this option.
If you have a better idea, please feel free to change that. By the
way, would you like me to revert this commit ?

I just would like to mention that sparse vectors allow a gain in
memory. Speed is, in my view, a bonus, but you probably think the
opposite...

Amitiés,
Sébastien


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message