Hey Bill,
I'm glad you looked at this! You point out something which is really
weird - as my comment in the JIRA ticket indicates, I believe I removed any
allowing of nonzero default values, for exactly the reasons you brought up
(in short, they're a total pain to deal with), and many more:
"New patch. This one has no reference to the "nonzero default values" for
sparse vectors, which can be addressed in another JIRA ticket. This patch
now only deals with the following files: "
At least I *thought* I'd reverted that change in the patch, and that's what
I was trying to submit. I have to look at the set of patches which were on
that ticket, and if neither of them were what I thought, then mea culpa,
that should have been removed (and I wish you'd responded to my comment in
the JIRA implying that I had already fixed it, and asking you if you were
using the most recent patch - had you replied saying 'yes I am, actually!',
I could have caught this and fixed it ages ago)!
On Wed, Dec 9, 2009 at 9:28 PM, Bill Barker <billwbar...@verizon.net> wrote:
> There are some things I don't like about this, but will try to find the
> time to fix them myself . Comments inline (and I can't veto anything in
> [math], so these are just suggestions).
>
> + /** Simple constructor. */
>> + protected SparseEntryIterator() {
>> + dim = getDimension();
>> + current = new EntryImpl();
>> + if (current.getValue() == 0) {
>> + advance(current);
>> + }
>>
>
> This totally doesn't work if the vector consists of all zero elements. The
> 'hasNext' method (below) will return true, and you will get an exception
> trying to get the first element.
Yes, this is a great unit test case, should have been checked for (and is a
special case, which should possibly be dealt with separately).
And actually, as I mention in the javadocs for this SparseIterator class,
this class has limited usefulness: when would *not* writing a specialized
implementation of iterateNonZero() be done in particular implementations of
RealVector? In Mahout's version of this class, the contract is a little
looser: iterateNonZero() doesn't try to gaurantee that it only iterates over
nonzero elements, it is really just the "sparseIterator" - default behavior
in the abstract base class is to delegate iterateNonZero() to iterate(), and
let subclasses decide whether there is something more sparse that can be
done. I think that's a lot cleaner, and avoids the dancing you have to do
to walk the AbstractRealVector sparsely without knowing what your
implementation is.
-jake
