nalgebra
nalgebra copied to clipboard
adds row and column iterators for sparse matrices
A nice convenience iterator that I end up using often. Iterates over (column_index, &T)
for CSR rows and (row_index, &T)
for CSC columns (and mutable counterparts).
Thanks, this looks great!
I only have a few minor comments related to documentation and naming. For example, maybe we should rename
CsrInnerIter
toCsrRowIter
andCscInnerIter
toCscColIter
? What do you think?Please see code comments for the other stuff.
This is actually what I wanted to originally name them but these names are used for the iterators over the lanes (outer iterators). So I wasn't sure what name would be next best.
Could possibly rename those outer iterators but then I'm not sure what would be a clear name for them. This is partially what motivated the inner
scheme because it clearly distinguishes it as iterating within the lane instead of over the lanes.
I do agree that the inner
scheme does also expect the user to know about the different compressed sparse matrix structures, like you mentioned that you wanted to avoid. I'll keep thinking for a better name...
@aujxn: ah, I had totally forgotten about the naming of "outer" iterators. Right, I suppose Inner
is not too bad then :-)
Yeah, in retrospect the naming of the outer iterators is maybe not ideal. Perhaps they should have been CsrRowsIter
and CscColsIter
? Not sure... In any case, it seems like a bad idea to directly "replace" CsrRowIter
with CsrInnerIter
, since the current CsrRowIter
may be directly named in user code, which would cause some very confusing error messages upon upgrade.
If we want to go ahead with a name change we should probably rather have a long term perspective:
- rename
CsrRowIter
toCsrRowsIter
- create deprecated type alias
CsrRowIter = CsrRowsIter
- in a (much) later release, in which
CsrRowIter
has already been deprecated for several releases, renameCsrInnerIter
toCsrRowIter
I'll create a separate issue to track this plan if we want to go ahead with it. In any case, it seems to me that we should keep the current naming that you've used in the current PR. What do you think?
I like the long term plan if you think it's worth it. Personally I use this pattern a lot and I've even seen it throughout the crate so it is nice to have. It might be better to wait to merge until CsrRowIter
is fully deprecated for a couple releases before merging? That way I can set this as a WIP with the naming that we eventually want to get to as well as fix the documentation and it can be ready for when the time comes.
@aujxn: this seems to have stalled, and that might have been partially my fault, I'm sorry. Do you have any interest in pushing this across the finishing line? I think it would make a nice addition to nalgebra-sparse
.