-
Notifications
You must be signed in to change notification settings - Fork 121
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
More MOIified implementation, again #504
Conversation
Status:
From a recent SCS run locally: Test Summary: | Pass Fail Error Total Time
SCS | 611 5 10 626 4m03.2s
constant | 28 28 4.4s
affine | 140 140 14.1s
exp | 27 27 4.8s
socp | 99 1 100 14.5s
lp | 56 2 58 4.5s
sdp | 258 5 7 270 3m18.8s |
Agree. If you want to see my recent work on this: |
@odow would you mind summarizing any tips or general strategy for that work? Specifically, do you have any tips for cases where the user passes input that influences things? E.g. in Convex the user can write |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #504 +/- ##
==========================================
+ Coverage 90.27% 92.00% +1.73%
==========================================
Files 84 87 +3
Lines 5572 5293 -279
==========================================
- Hits 5030 4870 -160
+ Misses 542 423 -119 ☔ View full report in Codecov by Sentry. |
I think I've made some good progress tightening up the |
Ok, I think this is in decent shape now; in particular the Warmstarts are not re-implemented, but I'm not sure it's essential for this PR. There is probably some missing test coverage that should be fixed before we merge however. I will mark it as ready-for-review now. |
I've been looking into perf since I believe this has regressed things. I also have some updates in #254. Running the benchmarks here, I get:
So we have regressed formulation for a bunch of problems, although with better memory consumption in many cases. Right now I maintain a lazy "tape" of affine operations to compose. (This came from the original PR #393). Trying without that, I get (against the same baseline), which looks like the right direction at least.
with some additional optimization (aad79b8), I get
|
@ericphanson what's the status of this ? I'd like to make some changes to improve the MOI wrapper support so it would be easier if at least part of this big PR is merged in order to avoid conflict. The small perf difference do not seem big enough to block merging |
Probably just needs some review and a rebase. If you are able to give some review that would help move it forward. I don’t remember there being any real blockers or big TODOs left |
aad79b8
to
63aa96c
Compare
@odow @ericphanson Any objection to merge this and follow the plan of my above comment ? Since this PR has such a large diff, it's going to be a pain to review changes on top of it and I'd like to try a few things. |
I think that seems OK, especially if you've been looking through the diff as you've worked here. I think that can effectively be some amount of review. At work I'd suggest we pair review it, where we get on a call and go through it together, but realistically I don't think I would have time anytime soon for that. Are there any areas you are concerned about? |
One thing I've been wondering is why allowing to pass an instantiated optimizer in |
src/problems.jl
Outdated
@@ -31,6 +30,8 @@ function Base.getproperty(p::Problem, s::Symbol) | |||
else | |||
return objective_value(p) | |||
end | |||
elseif s === :size | |||
return (1, 1) |
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'm not so sure what this is for as well. It actually wasn't tested so I had to add a test
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.
hm, if it was coming up in codecov as untested maybe it's not used at all and we can delete it. I don't really remember this, but if I had to guess it would be for something like partially specified problems (#310)
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.
Let's try to remove it and see if anything breaks:
5b401f0
Hm, that would make sense. I don't think I understood (and probably still don't understand) the semantics of optimizers and just messed with it until it seemed to work. Changing it seems totally fine. |
I have done the changes suggested in #504 (comment) and #504 (comment) and it looks good to me know, I'll merge next week if there is no objection |
This is too big to review. But given Convex.jl has seen very little activity recently, I'm okay releasing this as a breaking change if it is an improvement that fixes open issues. @mlubin should take a brief look before merging. |
I've added this PR to the agenda for next week's developer call. |
It looks like |
There's an open issue: #518 |
No, the plan to wait a while before releasing and at least fixing #518 |
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.
Ok to merge. (Comment can be addressed now or later.)
appropriate variables) in | ||
[solve!](https://github.com/jump-dev/Convex.jl/blob/master/src/solution.jl#L205). | ||
1. A `Problem{T}` struct is created by putting together an objective function and constraints. | ||
This forms a tree of sorts, in which variables and constants are the leaves, and atoms form the |
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.
Can we be more precise than "a tree of sorts"?
Developer call agrees to merge as a breaking change. @blegat plans a few other changes once this lands before we tag the next release. |
Updated #393 to the latest master, and made a few fixes and cleanup. Still has a ways to go.
UniqueConicForms
construction with similar-in-spiritContext
object, which now holds the MOI problem so we can add constraints/variables directly there as we traverse the problem. This is the main improvement which lets us lower new cones directly to MOI and handle things like geomean: Support for vectors #388.context
the first argument toconic_forms!
to make it clear that is what is being mutated (not the atom)conic_forms!
within a problem. We do so by simply not caching them. However I think if we wanted to, we could cache byobjectid
instead of hash, xref Proof of concept more-MOIified implementation #393 (comment).scaledgeomean
exported but not defined #392 by deleting the file