-
Notifications
You must be signed in to change notification settings - Fork 5
add specializations svd_trunc(!) for TruncatedAlgorithm
#171
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
🚀 New features to boost your workflow:
|
| # compute pullbacks | ||
| $f_pullback!(dA, Ac, DVc, dDVtrunc, ind) | ||
| zero!.(dDVtrunc) # since this is allocated in this function this is probably not required | ||
|
|
||
| # restore state | ||
| copy!(A, Ac) | ||
| copy!.(DV, DVc) |
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.
To be consistent with the other adjoints, the copy!(A, Ac) would happen before calling the pullback, and then you can just pass A in the f_pullback! call. I am not opposed to the current order/approach but do value consistency as it helps in reading the code at a later stage.
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 would like to do a follow up PR where I refactor a bunch of this into the same order and format, but didn't want to pollute this PR too much with simple formatting changes.
| # compute pullbacks | ||
| $f_pullback!(dA, Ac, DVc, dDVtrunc, ind) | ||
| zero!.(dDVtrunc) # since this is allocated in this function this is probably not required | ||
|
|
||
| # restore state | ||
| copy!(A, Ac) | ||
| copy!.(DV, DVc) |
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.
Same comment about order of operations.
|
I think I addressed most of the remarks here, except for the order of operations. Would you be okay with me leaving that for a follow-up PR? I would like to refactor some of this code a bit more, and I don't want to pollute this PR with a bunch of formatting changes if possible. |
No description provided.