On Sat, Jan 31, 2009 at 7:46 PM, Bill Barker <[email protected]> wrote:
> 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.
>
<snip/>
OK.
On a separate note, just noticed that the new files you added didn't
have any props -- svn:{eol-style,keywords} being the relatively
important ones. Can you check the auto-props section of your client
config?
-Rahul
> "Rahul Akolkar" <[email protected]> wrote in message
> news:[email protected]...
> 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, <[email protected]> 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
>>
>> ----- [email protected] 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: [email protected]
For additional commands, e-mail: [email protected]
