-
Notifications
You must be signed in to change notification settings - Fork 10
Remove allocations R2 #105
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
| # define model | ||
| φk(d) = dot(∇fk, d) | ||
| mk(d) = φk(d) + ψ(d) | ||
| mk(d)::R = φk(d) + ψ(d)::R |
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.
Still some allocations here but I could not fix it, maybe it is more related to ProximalOperators.
| mk(d) = φk(d) + ψ(d) | ||
| mk(d)::R = φk(d) + ψ(d)::R | ||
|
|
||
| prox!(s, ψ, mν∇fk, ν) |
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.
Some allocations here as well, maybe also related to ProximalOperators.
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.
Probably ShiftedProximalOperators?!
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.
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.
Are the allocations gone now?
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.
There are still a few (16 per iteration on line 280 and 282), I could not get lower on my PRs in ShiftedProximalOperators.
The biggest number of allocations comes from ψ = shifted(h, xk, l_bound_m_x, u_bound_m_x, selected) (as you mentionned below) at the begging of R2, but it is not inside a loop. We should think about a way of preallocating the ShiftedProximalOperators.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 59.26% 60.64% +1.38%
==========================================
Files 11 11
Lines 1225 1263 +38
==========================================
+ Hits 726 766 +40
+ Misses 499 497 -2
☔ View full report in Codecov by Sentry. |
|
Here are the |
| mk(d) = φk(d) + ψ(d) | ||
| mk(d)::R = φk(d) + ψ(d)::R | ||
|
|
||
| prox!(s, ψ, mν∇fk, ν) |
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.
Probably ShiftedProximalOperators?!
| @. u_bound_m_x = u_bound - xk | ||
| ψ = shifted(h, xk, l_bound_m_x, u_bound_m_x, selected) | ||
| else | ||
| ψ = shifted(h, xk) |
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.
There are also allocations here, right? We can think about it in a separate PR, but we should have an issue to track them.
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.
Yes, I forgot this. We leave it as it is for this PR as you propose, and maybe create a function shifted! in ShiftedProximalOperators to allow us to preallocate the ShiftedProximalOperator structure?
acc1e57 to
59a2590
Compare
|
Here are the |
No description provided.