-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add A.setdiag(x, k)
#493
Conversation
This is ready for review. I updated it to implement option 2 from #487 (comment) by adding The main remaining question I have is regarding 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 |
There was a problem hiding this 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.
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.
Add it as an issue to discuss, but don't hold up this PR for that. |
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? |
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 |
There was a problem hiding this comment.
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.
See: #487.
This adds
setdiag
method toMatrix
andUpdater
(such asA("+").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) orreplace=True
.Tests, documentation,feedback, and criticism are still needed.