It's a straight copy/paste from RealVectorImpl. I agree that public isn't best (but would probably go for protected instead of private so still usable by subclasses). However, making it non-public breaks the junit tests (which are a copy of the RealVectorImpl junit test with a search/replace, and some non-important one currently commented out). Would be +1 for changing checkVectorDimension(int) to protectected in both RealVectorImpl and SparseRealVector.
"Rahul Akolkar" <rahul.akol...@gmail.com> wrote in message news:ce1f2ea80901311124j3bef7aedsd4c60e82706a...@mail.gmail.com... Minor comment, and arguably subjective -- I find the signature for checkVectorDimensions(int) a little odd for a public method (I'd make it private). -Rahul On Sat, Jan 31, 2009 at 10:13 AM, <luc.maison...@free.fr> wrote: > Here are my comments. > > The two isZero methods seem inconsistent with each other. If I explicitely > set a component to epsilon/2 at index i, then isZero(i) would be false > (the element is set) but isZero(getEntry(i)) would be true. > > It would be nice to be able to set epsilon at construction. If for example > I want to build a vector from an array filtering out the small values, > currently I need to first build an empty vector, then set epsilon, then > fill the vector with my array. > > In the optimized version of add(SparseRealVector), I wonder if the call to > res.set which either calls entries.put or entries.remove could not > invalidate the iterator. After a quick look, it seems OK > (findInsertionIndex in OpenIntToDoubleHashMap should always return a > negative value and newMapping should be always reset to false), but I'm > not sure and this seems to really be implementation dependent. > > In the add, subtract and ebeXxx methods, the result vector is first built > as a copy of the instance and later all its elements are overwritten. The > initial copy could probably be avoided by simply iterating on the instance > by itself and setting the elements of an initially empty vector, built > with the advanced use constructor with both expected size and dimension. > > This is a nice work, thanks > Luc > > ----- billbar...@apache.org a écrit : > >> Author: billbarker >> Date: Sat Jan 31 04:51:17 2009 >> New Revision: 739504 >> >> URL: http://svn.apache.org/viewvc?rev=739504&view=rev >> Log: >> Initial checkin for the SparseRealVectorClass. >> >> I know that it doesn't work 100% with the map*** methods that >> shouldn't be used with a sparse vector. I'll clean those up shortly >> (including uncommenting unit tests). Just want to get more eyes on >> this for the methods that matter. >> <snip/> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
