Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
eriknw merged 12 commits intopython-graphblas:mainfromeriknw:wip_setdiag
Sep 22, 2023
Merged

AddA.setdiag(x, k)#493

eriknw merged 12 commits intopython-graphblas:mainfromeriknw:wip_setdiag
Sep 22, 2023

Conversation

@eriknw
Copy link
Member

@eriknweriknw commentedAug 23, 2023
edited
Loading

See:#487.

This addssetdiag method toMatrix andUpdater (such asA("+").setdiag(v)).setdiag is not available on transposed matrices or expressions.setdiag is 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 forcomplemented masks (complemented masks now implemented) orreplace=True.

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

@eriknweriknw marked this pull request as draftAugust 23, 2023 03:26
@coveralls
Copy link

coveralls commentedAug 23, 2023
edited by github-actionsbot
Loading

Coverage Status

coverage: 98.838% (-0.9%) from 99.743% when pulling9e51b87 on eriknw:wip_setdiag intobf28a79 on python-graphblas:main.

@eriknweriknw changed the titleExperiment with addingA.setdiag(x, k)AddA.setdiag(x, k)Sep 12, 2023
@eriknweriknw marked this pull request as ready for reviewSeptember 12, 2023 06:27
@eriknweriknw added enhancementImprove existing functionality or make things work better featureSomething is missing labelsSep 12, 2023
@eriknw
Copy link
MemberAuthor

This is ready for review. I updated it to implement option 2 from#487 (comment) by addingmask= andaccum= arguments tosetdiag.

The main remaining question I have is regardingclear_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 addingmatrix.setdiag for reasons stated in that comment.

CC@q-aaronzolnailucas@SultanOrazbayev@jim22k

Copy link
Member

@jim22kjim22k 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 allowA(mask, accum=accum).setdiag(v)? Which would just delegate to the keywords in thesetdiag method.

@eriknw
Copy link
MemberAuthor

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

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
MemberAuthor

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 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?

Comment on lines +2914 to +2919
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
Copy link
MemberAuthor

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.

SultanOrazbayev reacted with thumbs up emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jim22kjim22kjim22k approved these changes

Assignees

No one assigned

Labels

enhancementImprove existing functionality or make things work betterfeatureSomething is missing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eriknw@coveralls@jim22k

[8]ページ先頭

©2009-2025 Movatter.jp