Bill Barker a écrit : > 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.
+1. The original signature is a mistake I made. Luc > > "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 > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
