Hello Sébastien. > >> + /* > >> + * 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).
The problem is when the common values are not the default one, like ... > > > 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". ... in this example. > > > >> @@ -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 ? No. But maybe we should already open a ticket signalling that a potential problem has been identified. > > 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... No, that's fine; less so if you lose both. ;-) Best regards, Gilles --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org