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

Add Objective bridges #789

Merged
merged 5 commits into from
Sep 5, 2019
Merged

Add Objective bridges #789

merged 5 commits into from
Sep 5, 2019

Conversation

blegat
Copy link
Member

@blegat blegat commented Jul 13, 2019

Note that the base branch set is bl/variable_bridge but I don't plan to merge it into it, it's only set to have a small diff. This PR will only be merged once bl/variable_bridge is merged into master.

Closes #529

@blegat blegat added Type: Enhancement Submodule: Bridges About the Bridges submodule labels Jul 13, 2019
@blegat blegat force-pushed the bl/variable_bridge branch 4 times, most recently from bd63ad3 to 20a0265 Compare July 22, 2019 01:36
@blegat blegat force-pushed the bl/variable_bridge branch 6 times, most recently from ecc3c56 to b9ca024 Compare August 6, 2019 07:47
@blegat blegat force-pushed the bl/variable_bridge branch 8 times, most recently from 98d020b to 06ab49a Compare August 9, 2019 20:00
@chriscoey
Copy link
Contributor

So this is for converting unsupported objective types to supported constraints? like quadratic obj to quadratic constraint? max logdet to max of a new variable and a logdet cone constraint? any weird use cases planned?

@blegat
Copy link
Member Author

blegat commented Aug 10, 2019

So this is for converting unsupported objective types to supported constraints? like quadratic obj to quadratic constraint? max logdet to max of a new variable and a logdet cone constraint? any weird use cases planned?

Yes, exactly log-det (that is supported by SDPT3 and should be bridged for others) and quadratic objectives which is suppoted by some solvers and should be transformed to RSOC are the main use cases.
The diff is abnormally large since it is based on bl/variable_bridges which has changed quite a lot since when it was written

@chriscoey
Copy link
Contributor

cool. so the objective bridges are aware of the objective sense? like can it check whether the sense is compatible with reformulating using a convex epi/hypograph constraint?

@blegat
Copy link
Member Author

blegat commented Aug 10, 2019

so the objective bridges are aware of the objective sense?

That's the most tricky part with objective bridges, it would have been easier if both sense and objective are set at the same times.
So yes, they are aware of objective sense but there are tricky cases that do not work now, e.g.

set sense to Min
set function to convex quad
set sense to Max # error
set function to concave quad

@chriscoey
Copy link
Contributor

seems like it would make sense to set an objective sense and function at same time. how breaking would it be to change the interface?

@blegat
Copy link
Member Author

blegat commented Aug 10, 2019

It might be quite breaking. We might start with an error as this use case it not that common and evaluate if it's worth the breakage.

@chriscoey
Copy link
Contributor

I'm thinking of cases like https://github.com/JuliaOpt/Pajarito.jl/blob/master/examples/expdesign.jl where you have different convex objectives some of which are "min convex function" and some that are "max concave function" and you want to switch between the objectives easily to see how they affect the solution. Or cases where you try different convex objectives for regression problems, some that are max and some min.

@mlubin
Copy link
Member

mlubin commented Aug 10, 2019

seems like it would make sense to set an objective sense and function at same time. how breaking would it be to change the interface?

This would be a usability loss for the API without a clear justification. Why can't we look at the combination of objective sense and objective function and then decide which transformation to apply?

@blegat
Copy link
Member Author

blegat commented Aug 10, 2019

Why can't we look at the combination of objective sense and objective function and then decide which transformation to apply?

That's another approach, cache the objective function, in case the bridge is sensitive to min vs max, cache the objective function until optimize! is called hence you are sure to have the sense set

@rschwarz
Copy link
Contributor

From the user's point of view, when switching from "min convex" to "max concave", couldn't we clear the objective by switching to feasibility mode as an intermediate step?

@blegat blegat marked this pull request as ready for review August 29, 2019 14:01
@blegat blegat changed the base branch from bl/variable_bridge to master August 29, 2019 14:02
@blegat blegat force-pushed the bl/objective_bridge branch 2 times, most recently from e7f21a7 to 61b22cc Compare August 29, 2019 16:48
@blegat blegat added this to the v0.9.2 milestone Sep 1, 2019
@blegat
Copy link
Member Author

blegat commented Sep 1, 2019

Adding to the v0.9.2 milestone as currently, bridged variables cannot be used in SingleVariable objective which is a regression as SDOI could handle that. This PR fixes this issues by force-bridging the SingleVariable to a ScalarAffineFunction with Bridges.Objective.FunctionizeBridge

@blegat blegat force-pushed the bl/objective_bridge branch 2 times, most recently from bf29a46 to e3f6c93 Compare September 1, 2019 10:13
@ericphanson
Copy link
Contributor

Adding to the v0.9.2 milestone as currently, bridged variables cannot be used in SingleVariable objective which is a regression as SDOI could handle that. This PR fixes this issues by force-bridging the SingleVariable to a ScalarAffineFunction with Bridges.Objective.FunctionizeBridge

Just to say @JiazhengZhu ran into this when trying to test SDPA-GMP with some JuMP examples.

@blegat blegat force-pushed the bl/objective_bridge branch 2 times, most recently from ecded8e to 2ec0202 Compare September 3, 2019 15:46
@codecov-io
Copy link

codecov-io commented Sep 3, 2019

Codecov Report

Merging #789 into master will increase coverage by 0.09%.
The diff coverage is 91.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #789      +/-   ##
==========================================
+ Coverage   94.87%   94.97%   +0.09%     
==========================================
  Files          77       80       +3     
  Lines        8094     8274     +180     
==========================================
+ Hits         7679     7858     +179     
- Misses        415      416       +1
Impacted Files Coverage Δ
src/Bridges/Variable/single_bridge_optimizer.jl 30% <0%> (-3.34%) ⬇️
src/Bridges/Constraint/single_bridge_optimizer.jl 40% <0%> (-4.45%) ⬇️
src/Bridges/Objective/slack.jl 100% <100%> (ø)
src/Utilities/model.jl 93.55% <100%> (+0.05%) ⬆️
src/Bridges/Objective/Objective.jl 100% <100%> (ø) ⬆️
src/Bridges/Bridges.jl 100% <100%> (ø) ⬆️
src/Bridges/Objective/single_bridge_optimizer.jl 30% <30%> (ø)
src/Bridges/lazy_bridge_optimizer.jl 92.9% <92.06%> (+2.18%) ⬆️
src/Bridges/Objective/functionize.jl 94.11% <94.11%> (ø)
src/Bridges/bridge_optimizer.jl 95.59% <95.89%> (-0.83%) ⬇️
... and 8 more

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 a3ee2fb...98f5063. Read the comment docs.

@blegat
Copy link
Member Author

blegat commented Sep 3, 2019

This PR should be ready now. The SlackBridge needs the objective sense to be set before the objective function and does not support the objective sense to be changed. A workaround is to set it to FeasibilitySense first to remove the bridge in order to change it from max to min or min to max. At a later stage, we could cache the objective and sense to and only set it before optimize but that's out of scope for this PR.

I have created a PR with an independent subset of the changes for easier review: #867

src/Bridges/Constraint/det.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/det.jl Outdated Show resolved Hide resolved
"Objective bridge of type `$(typeof(bridge))` does not support" *
" modifying the objective sense. As a workaround, set the sense to" *
" `MOI.FEASIBILITY_SENSE` to clear the objective function and" *
" bridges."))
Copy link
Member

Choose a reason for hiding this comment

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

So what is JuMP going to do? Is it possible to easily set FEASIBILITY_SENSE from JuMP?

Copy link
Member Author

Choose a reason for hiding this comment

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

The user can do set_objective_sense(model, MOI.FEASIBILITY_SENSE). This is not ideal but this is a rare use case. We can implement #789 (comment) if this is an issue

src/Bridges/Objective/bridge.jl Outdated Show resolved Hide resolved
src/Bridges/Objective/bridge.jl Outdated Show resolved Hide resolved
src/Bridges/Objective/map.jl Outdated Show resolved Hide resolved
src/Bridges/Objective/slack.jl Outdated Show resolved Hide resolved
src/Bridges/bridge.jl Outdated Show resolved Hide resolved
src/Bridges/bridge_optimizer.jl Show resolved Hide resolved
return MOI.supports(b.model, attr)
end
end
struct ObjectiveFunctionValue{F<:MOI.AbstractScalarFunction} end
Copy link
Member

Choose a reason for hiding this comment

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

Why is it necessary to have a different attribute? What's the difference between ObjectiveFunctionValue and ObjectiveValue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a docstring

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

I guess #870 is going to be merged first, and then this rebased on-top?

src/Bridges/Objective/functionize.jl Show resolved Hide resolved
src/Bridges/Objective/functionize.jl Show resolved Hide resolved
@blegat blegat merged commit 8d971fb into master Sep 5, 2019
@blegat blegat deleted the bl/objective_bridge branch October 10, 2019 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Submodule: Bridges About the Bridges submodule Type: Enhancement
Development

Successfully merging this pull request may close these issues.

Objective function bridge
7 participants