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

Remove error from feasibility sense #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guilhermebodin
Copy link
Collaborator

@guilhermebodin guilhermebodin commented Jun 2, 2023

close #155

@blegat This makes it work but I am not sure what is the formal dual of a feasibility problem.

If we assume feasibility is Min 0 should the dual be the same as if we assume Max 0 or maybe it's whatever. Also, the dual of the dual would not be a feasibility-sense problem.

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.74%. Comparing base (14edeae) to head (c333945).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #162   +/-   ##
=======================================
  Coverage   94.73%   94.74%           
=======================================
  Files          13       13           
  Lines         703      704    +1     
=======================================
+ Hits          666      667    +1     
  Misses         37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odow
Copy link
Member

odow commented Jun 2, 2023

If we assume feasibility is Min 0 should the dual be the same as if we assume Max 0 or maybe it's whatever. Also, the dual of the dual would not be a feasibility-sense problem.

Devils advocate: does a feasibility sense problem have a dual? It doesn't have an objective function.

@joaquimg
Copy link
Member

joaquimg commented Jun 2, 2023

I would say there is no dual.
If someone wants a dual, select a sense. Coefficients may remain zero in the objective.

@guilhermebodin
Copy link
Collaborator Author

I agree @odow. The other sane option would be to make a more informative error message such as:

error(primal_sense, " does not have a well-defined dual problem. A solution for this problem is to add the objective `Min 0` and assume that it is a minimization problem.")

@odow
Copy link
Member

odow commented Jun 2, 2023

Yeah a more informative error sounds good.

@blegat
Copy link
Member

blegat commented Jun 2, 2023

If a user solve a conic problem, he should always try with and without Dualization because it can change lot (it also impacts the low-rank-ness of the solution which is important for instance for Burer-Monteiro-like and ProxSDP). By the way, we should insist more about that in the JuMP doc. I'm going to do it in the SumOfSquares block but this issue is blocking it.
Having Dualization fail for FEASIBILITY_SENSE is a big blocker.
In MOI, the dual for max sense is defined as the dual of min sense where the objective is reversed so there should be no difference in which sense we choose here (but I would choose MIN_SENSE)

@odow
Copy link
Member

odow commented Jun 2, 2023

Having Dualization fail for FEASIBILITY_SENSE is a big blocker.

Why? The user could just set a min 0 objective. SumOfSquares could do so too.

@blegat
Copy link
Member

blegat commented Jun 4, 2023

Why? The user could just set a min 0 objective. SumOfSquares could do so too.

That wouldn't really be an option because SumOfSquares just defines new MOI sets and MOI bridges.

The dual of MAX_SENSE is really just defined in MOI as the dual of MIN_SENSE with the reversed objective except that the dual is with a MIN_SENSE and not a MAX_SENSE
Here, because the objective is zero, reversing it does not change anything so the only thing that would be different is that the dual would be a minimum or a maximum. I think defaulting on min 0 and having a max in the dual is a safe choice.

Another option is to have a keyword in dualize so that if you use dualize without a sense and without setting that keyword you get an error.
Then Dualization.Optimizer would set that keyword to default to min because the users won't see the difference because the dualization is transparant in the solution process.

@odow
Copy link
Member

odow commented Jun 4, 2023

What was the upstream bug? https://github.com/jump-dev/SumOfSquares.jl/pull/283/files

You tried to do

import Dualization, SCS
model = SOSModel(() -> Dualization.Optimizer(SCS.Optimizer))
@constraint(model, motzkin >= 0)
optimize!(model)

and it didn't work because no objective was set? Then either SOSModel could always start by setting min 0 as an objective, or you could say to the user "hey, set an objective."

It currently doesn't work, so it isn't breaking to change an error message.

@blegat
Copy link
Member

blegat commented Jun 6, 2023

jump-dev/SumOfSquares.jl#283 is not about a breaking change in Dualization. The tutorial started failing because of the bug that was fixed in jump-dev/CSDP.jl#89 so I tried SCS but I couldn't because dualization does not support feasibility sense so I went back to using CSDP since I fixed the bug in CSDP.

I understand that choosing this objective sense in dualize is confusing since it affects the objective sense of the dual which is in the dualized problem.
However, for Optimizer, it really shouldn't be a problem. It's goal is to transparently solve the dual problem in the solver. The objective sense given in the solver really shouldn't matter. If you don't want to add a keyword to the dualize function, then Optimizer can also add a filter layer that will change the getters of ObjectiveSense and ObjectiveFunction like in https://github.com/ericphanson/SDPAFamily.jl/pull/66/files#diff-33ec7fe2aadeaed51062fede6400fae5468eda2717c631927e70a184e3b27ccd

and it didn't work because no objective was set? Then either SOSModel could always start by setting min 0 as an objective, or you could say to the user "hey, set an objective."

SOSModel is only needed if you want to use the >= syntax otherwise you can just use Model. Now that we have @constraint(model, .. >= ..., PSDCone()) I could actually get rid of it in favor of @constraint(model, ... >= ..., SOSCone()) so Model should be enough. So I don't have any opportunity to add an objective.
Also, for the same reason, it's weird to add an objective in Dualization, it is even weirder in SumOfSquares.
SumOfSquares has nothing to do with this. Dualization needs an objective to be set and Dualization is important for conic problems and SumOfSquares creates conic problems but doing this in SumOfSquares looks like a weird hack.

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.

Support feasibility sense
4 participants