-
Notifications
You must be signed in to change notification settings - Fork 5
Support for Diagonal
#50
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
Conversation
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
6e119a5 to
0a8df6b
Compare
| end | ||
|
|
||
| p .+= (0:(n - 1)) .* n | ||
| U[p] .= Ref(one(eltype(U))) |
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.
I don't think this Ref is necessary due to the broadcasting behaviour of scalars, but I assume it also has no cost. Other than this, I have no comments and think this looks great.
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.
Semi related, why not use @. broadcast syntax to minimize the number of array iterations?
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.
I think I was thinking when implementing this that since the Diagonal implementation really requires very little from the scalars, it might be that someone at some point uses this for some exotic type that actually has some unwanted broadcasting behavior, so better to be explicit about it. Realistically though, this totally doesn't matter 🙃
Here I've added specialized implementations for working with
Diagonals, through a dedicated backend.