- Notifications
You must be signed in to change notification settings - Fork15
AddA.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedAug 23, 2023 • edited by github-actionsbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by github-actionsbot
Uh oh!
There was an error while loading.Please reload this page.
eriknw commentedSep 12, 2023
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 |
jim22k left a comment
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 allowA(mask, accum=accum).setdiag(v)? Which would just delegate to the keywords in thesetdiag method.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
eriknw commentedSep 14, 2023
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 commentedSep 14, 2023
Add it as an issue to discuss, but don't hold up this PR for that. |
eriknw commentedSep 15, 2023
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 setany 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) | ||
| ifDiag.shape!=self.shape: | ||
| Diag.resize(self._nrows,self._ncols) | ||
| ifmaskisNone: | ||
| 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.
Uh oh!
There was an error while loading.Please reload this page.
See:#487.
This adds
setdiagmethod toMatrixandUpdater(such asA("+").setdiag(v)).setdiagis not available on transposed matrices or expressions.setdiagis doing anassignment, 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.