Hi Gilles, 2012/6/11 Gilles Sadowski <gil...@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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org