|Summary:||InnerIterator cannot be assigned, copy- or default-constructed|
|Product:||Eigen||Reporter:||Clemens Hofreither <clemens.hofreither>|
|Bug Depends on:|
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
Comment 9 Nobody 2019-12-04 12:06:35 UTC
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/557.