New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 557 - InnerIterator cannot be assigned, copy- or default-constructed
InnerIterator cannot be assigned, copy- or default-constructed
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Sparse
unspecified
All All
: Normal enhancement
Assigned To: Nobody
:
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2013-02-21 15:07 UTC by Clemens Hofreither
Modified: 2016-02-01 14:08 UTC (History)
2 users (show)



Attachments
A patch which fixes all described issues. (771 bytes, text/plain)
2013-02-21 15:13 UTC, Clemens Hofreither
no flags Details
A possible patch (37.71 KB, patch)
2013-06-28 23:25 UTC, Gael Guennebaud
no flags Details | Diff

Description Clemens Hofreither 2013-02-21 15:07:44 UTC
InnerIterator doesn't have a default constructor, a copy constructor and an assigment operator. Usually, C++ would generate the last two by itself, but it can't because the class has non-static const members.

This precludes the following use cases:
*) arrays of InnerIterators: doesn't work, no default constructor
*) std::vectors of InnerIterators: doesn't work, no assignment operator

The simplest workaround is probably to provide a default constructor and make its members non-const so that C++ can generate the other two methods.
Comment 1 Clemens Hofreither 2013-02-21 15:13:54 UTC
Created attachment 315 [details]
A patch which fixes all described issues.

This patch implements my proposed changes.
Comment 2 Desire NUENTSA 2013-02-22 16:22:16 UTC
Good Stuff, 
at least the default constructor is needed.
What do you think about this Gael ?
Comment 3 Gael Guennebaud 2013-02-25 18:19:57 UTC
I'm not sure it's a good idea to store a vector of iterators. InnerIterators are quite heavy. Could you elaborate on your use case? Maybe a better alternative would be to add the possibility to increment an InnerIterator of an arbitrary number. This way, you would only have to store a vector of indices.
Comment 4 Clemens Hofreither 2013-02-26 10:09:01 UTC
My use case is sound. I have a tensor grid, of statically known but arbitrarily large tensor dimension d. To each tensor direction i=1,...,d I have an associated sparse matrix. I want to compute an output matrix whose rows are, roughly speaking, (sparse) tensor products of the sparse rows of the input matrices.

The way I do this is (after applying the above patch to Eigen) by storing a statically sized array of size d of InnerIterators; each one points initially to the first non-zero item in a row of the associated input matrix. Then I advance them in lexicographic order, i.e., I first advance the first iterator, if it is exhausted, I reset it and advance the next one, etc. This works very nicely with the above patch. I ended up not using std::vectors of InnerIterators, but I see no reason not to allow them.

I see no problems with the patch since it doesn't interfere with any previous use cases and enables new ones. I don't know what you mean by "increment an InnerIterator of an arbitrary number".
Comment 5 Gael Guennebaud 2013-02-26 11:23:40 UTC
OK, thanks for the explanations. After all, why not. Just be aware such iterators are quite heavy (32B) while their essential information (the number of non-zeros that have been processed) fit in an integer (4B). So we could also offer an API like:

InnerIterator it(mat, j);
// ...
int p = it.pos(); // return the position of the iterator

InnerIterator it(mat, j);
it+=p; // jump to the previous position

I admit it's a bit less convenient to write in your case because you have to recreate an iterator everytime, but memory wise it's much more satisfactory because you avoid storing redundant information.

Nevertheless, I guess that both features can coexist.
Comment 6 Clemens Hofreither 2013-02-26 14:37:28 UTC
In my case I'm not worried about the storage requirements since I keep this data only on the stack for the duration of the algorithm described, which is only a small part of my overall program. Also, d will typically be small, say <= 5, so the overhead is negligible.

The only thing that would be a small optimization in my use case would be to provide a method which resets the iterator to the start of the current row, since then I would save recreating a new iterator. But I don't know if this can be implemented without adding additional data members to the iterator. Anyway, the algorithm is not a bottleneck in my code, so it's not very important to me.
Comment 7 Gael Guennebaud 2013-06-28 23:25:20 UTC
Created attachment 362 [details]
A possible patch

Finally, I'm not convinced we want them to be copyable. Just for the record I attached a possible patch. Let's postpone the resolution of this discussion.
Comment 8 Gael Guennebaud 2016-02-01 14:08:38 UTC
As a compromise, I eventually implemented this proposal for sparse-storage types (not for any expression):

https://bitbucket.org/eigen/eigen/commits/c00b92176825/
Summary:     Bug 557: make InnerIterator of sparse storage types more versatile by adding default-ctor, copy-ctor/assignment

Note You need to log in before you can comment on or make changes to this bug.