nalgebra icon indicating copy to clipboard operation
nalgebra copied to clipboard

adds row and column iterators for sparse matrices

Open aujxn opened this issue 2 years ago • 4 comments

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).

aujxn avatar May 03 '22 20:05 aujxn

Thanks, this looks great!

I only have a few minor comments related to documentation and naming. For example, maybe we should rename CsrInnerIter to CsrRowIter and CscInnerIter to CscColIter? 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 avatar Jun 23 '22 03:06 aujxn

@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:

  1. rename CsrRowIter to CsrRowsIter
  2. create deprecated type alias CsrRowIter = CsrRowsIter
  3. in a (much) later release, in which CsrRowIter has already been deprecated for several releases, rename CsrInnerIter to CsrRowIter

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?

Andlon avatar Jun 23 '22 06:06 Andlon

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 avatar Jun 23 '22 17:06 aujxn

@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.

Andlon avatar Dec 13 '22 08:12 Andlon