Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more documentation on array iteration #10902

Merged
merged 1 commit into from
Apr 20, 2015
Merged

Add more documentation on array iteration #10902

merged 1 commit into from
Apr 20, 2015

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Apr 19, 2015

Following up on suggestion for further documentation in #10858.

@tkelman
Copy link
Contributor

tkelman commented Apr 19, 2015

Cool. As an aside, I'm wondering if for the purposes of #8001 it might make sense to add an eachnonzero version of this iterator that's specialized for sparse (or banded, tridiag, etc) matrices, then reductions or other operations that are invariant with respect to implicit zeros can be written in terms of eachnonzero instead of eachindex...

@timholy
Copy link
Sponsor Member Author

timholy commented Apr 19, 2015

It's an interesting idea, but #6769 made the prospect of asking "what is a nonzero?" absolutely terrifying. Someone braver than me will have to implement it 😄.

@toivoh
Copy link
Contributor

toivoh commented Apr 19, 2015

eachstored?

@timholy
Copy link
Sponsor Member Author

timholy commented Apr 19, 2015

eachstored could work. Upon thinking about it, there doesn't seem to be a generic implementation: each "sparse" matrix type would have to implement its own.

timholy added a commit that referenced this pull request Apr 20, 2015
Add more documentation on array iteration
@timholy timholy merged commit a989d36 into master Apr 20, 2015
@timholy timholy deleted the teh/indexingdoc branch April 20, 2015 00:40
@tkelman
Copy link
Contributor

tkelman commented Apr 20, 2015

Really #6769 is more noise than substance. Anyone who actually works with sparse matrices regularly is perfectly comfortable with allowing explicitly stored zeros. A structural/stored nonzero can have a value equal to zero, and it shouldn't change any calculation results if things are implemented correctly.

Upon thinking about it, there doesn't seem to be a generic implementation: each "sparse" matrix type would have to implement its own.

That's exactly the point. Better than re-writing the same messy structure-specific iteration code inside every single method you want to implement for each matrix type. I'd rather write the iteration code once per type and use that everywhere, than have to duplicate it ntypes * nmethods times.

@toivoh
Copy link
Contributor

toivoh commented Apr 20, 2015

Still, I'm not sure if you really want to use reading and assignment to A[I] as the idiomatic way to operate on sparse matrices independent of type (where I is aCartesianIndex or some sparse matrix analogue). Would that really be efficient? Which kind of abstraction would we want to support for sparse matrices? What would make for efficient code?

@tkelman
Copy link
Contributor

tkelman commented Apr 20, 2015

(We could move this discussion elsewhere? #8001 maybe?) For CSC as an example, the analog to CartesianIndex would need to store column index and nonzero index. Reading from or writing to an existing nonzero can be efficient in that case. The inefficient operations would be inserting a new nonzero that was not previously allocated in the structure, or removing an existing nonzero from the structure. There would be a slight ambiguity in whether A[I] = 0 means "set the existing stored value to zero" or "remove the stored value at index I from the structure and replace it with an implicit zero," but I think for efficiency it would have to mean the former. The latter operation should be given a different name.

@toivoh
Copy link
Contributor

toivoh commented Apr 20, 2015

I agree with all of the above, and #8001 seems like a good place to continue the discussion.

@timholy
Copy link
Sponsor Member Author

timholy commented Apr 20, 2015

Also agreed, @tkelman, including the move of future discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants