Skip to content
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

matrix-less dQ #59

Merged
merged 11 commits into from
Jan 28, 2021
Merged

matrix-less dQ #59

merged 11 commits into from
Jan 28, 2021

Conversation

matbesancon
Copy link
Collaborator

In backward_conic, dQ is materialized only for a matrix-vector product, it is eliminated from the pass.

There should also be a way to avoid materializing Q, and ultimately reasoning on the sparse construction of the problem and not its explicit matrix form

@codecov
Copy link

codecov bot commented Jan 22, 2021

Codecov Report

Merging #59 (604cbda) into master (e569e78) will decrease coverage by 0.38%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   91.77%   91.39%   -0.39%     
==========================================
  Files           4        4              
  Lines         304      302       -2     
==========================================
- Hits          279      276       -3     
- Misses         25       26       +1     
Impacted Files Coverage Δ
src/DiffOpt.jl 100.00% <ø> (ø)
src/MOI_wrapper.jl 88.66% <92.30%> (-0.17%) ⬇️
src/utils.jl 95.00% <94.11%> (-1.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e569e78...604cbda. Read the comment docs.

@matbesancon
Copy link
Collaborator Author

Good to merge

@matbesancon matbesancon reopened this Jan 25, 2021
@matbesancon matbesancon requested a review from blegat January 27, 2021 15:14
@matbesancon matbesancon requested a review from joaquimg January 27, 2021 16:06
@matbesancon
Copy link
Collaborator Author

if someone wants to have a final look before merging this

end

du, dv, dw = dz[1:n], dz[n+1:n+m], dz[n+m+1]
@inbounds (du, dv, dw) = (dz[1:n], dz[n+1:n+m], dz[n+m+1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This @inbounds may trigger more segfault (when we change this function and forget something) than have any visible effect on perf no ? This isn't a critical part of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes right, normally we control the size of dz deduced from the RHS we form but the bound check might be more future-proof

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@matbesancon matbesancon merged commit 7c24003 into master Jan 28, 2021
@matbesancon matbesancon deleted the matrix-multiply branch January 28, 2021 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants