Skip to content

Add A.setdiag(x, k) #493

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

Merged
merged 12 commits into from
Sep 22, 2023
Merged

Add A.setdiag(x, k) #493

merged 12 commits into from
Sep 22, 2023

Conversation

eriknw
Copy link
Member

@eriknw eriknw commented Aug 23, 2023

See: #487.

This adds setdiag method to Matrix and Updater (such as A("+").setdiag(v)). setdiag is not available on transposed matrices or expressions. setdiag is doing an assignment, but with functional syntax instead of with __setitem__, so it is most similar to setting a Vector or scalar to a single row or column of a Matrix, so we follow the same semantics.

This supports masking and accumulation, but punts for complemented masks (complemented masks now implemented) or replace=True.

Tests, documentation, feedback, and criticism are still needed.

@eriknw eriknw marked this pull request as draft August 23, 2023 03:26
@coveralls
Copy link

coveralls commented Aug 23, 2023

Coverage Status

coverage: 98.838% (-0.9%) from 99.743% when pulling 9e51b87 on eriknw:wip_setdiag into bf28a79 on python-graphblas:main.

@eriknw eriknw changed the title Experiment with adding A.setdiag(x, k) Add A.setdiag(x, k) Sep 12, 2023
@eriknw eriknw marked this pull request as ready for review September 12, 2023 06:27
@eriknw eriknw added enhancement Improve existing functionality or make things work better feature Something is missing labels Sep 12, 2023
@eriknw
Copy link
Member Author

eriknw commented Sep 12, 2023

This is ready for review. I updated it to implement option 2 from #487 (comment) by adding mask= and accum= arguments to setdiag.

The main remaining question I have is regarding clear_missing=True argument. Does it make sense? Is the name good? Or should it not be included at all? Also, are the code and docstrings clear enough, or should any more docs be added?

I think the pros and cons from #487 (comment) are still accurate and I think are worth rereading when reviewing this PR. I am +1 for adding matrix.setdiag for reasons stated in that comment.

CC @q-aaronzolnailucas @SultanOrazbayev @jim22k

Copy link
Member

@jim22k jim22k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to allow A(mask, accum=accum).setdiag(v)? Which would just delegate to the keywords in the setdiag method.

@eriknw
Copy link
Member Author

eriknw commented Sep 14, 2023

Do you plan to allow A(mask, accum=accum).setdiag(v)? Which would just delegate to the keywords in the setdiag method.

Unsure. I removed this. It would be very easy to add back. What do you think?

Use `second` as the accumulator for `clear_missing=False` behavior.
@jim22k
Copy link
Member

jim22k commented Sep 14, 2023

Do you plan to allow A(mask, accum=accum).setdiag(v)? Which would just delegate to the keywords in the setdiag method.

Unsure. I removed this. It would be very easy to add back. What do you think?

Add it as an issue to discuss, but don't hold up this PR for that.

@eriknw
Copy link
Member Author

eriknw commented Sep 15, 2023

For a 0-length vector where k == 0, I could maybe see an exception. But if k != 0 and the off-diagonal is length zero, that is out of range. I think raising is the correct approach.

This (allowing to set 0-length when k == 0) is actually what setdiag in scipy.sparse does. I think this is a good idea and let's us better handle length 0 dimensions. For example, should it be possible to set any diagonal in a 0x2 Matrix? My latest commit allows this.

For the record, scipy.sparse setdiag also allows the size of the vector of new values to be too large or too small. Should we follow suite, or require that the vector exactly match the size of the diagonal, which is what we currently do?

Comment on lines +2914 to +2919
Diag = v.diag(k)
if Diag.shape != self.shape:
Diag.resize(self._nrows, self._ncols)
if mask is None:
mask = Diag.S
self(accum=accum, mask=mask, **opts) << Diag
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity and those who browse this PR, these lines are the main recipe for setting the diagonal of a Matrix with a Vector. The rest of the function mostly deals with handing different input types and giving good error messages when necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or make things work better feature Something is missing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants