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

[docs] add Dualization tutorial #3402

Merged
merged 19 commits into from
Jun 7, 2023
Merged

[docs] add Dualization tutorial #3402

merged 19 commits into from
Jun 7, 2023

Conversation

odow
Copy link
Member

@odow odow commented Jun 5, 2023

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (19ab5d6) 98.05% compared to head (518fc28) 98.05%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3402   +/-   ##
=======================================
  Coverage   98.05%   98.05%           
=======================================
  Files          34       34           
  Lines        4926     4926           
=======================================
  Hits         4830     4830           
  Misses         96       96           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

docs/make.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@jd-foster jd-foster left a comment

Choose a reason for hiding this comment

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

Some small tweaks.

I'll leave you to it: the commit was me not understand the github editor.

docs/src/tutorials/conic/dualization.jl Outdated Show resolved Hide resolved
@odow odow force-pushed the od/dualization branch from 786227e to d6cd523 Compare June 5, 2023 10:43
@odow

This comment was marked as outdated.

@odow

This comment was marked as resolved.

@odow
Copy link
Member Author

odow commented Jun 5, 2023

Pushed a local fix here. But I'll fix upstream SCS.jl

@odow
Copy link
Member Author

odow commented Jun 6, 2023

So I'm going to push back on the suggestion that we should use Dualization in other tutorials. This is an extra for experts thing, and the addition of Dualization.dual_optimizer distracts from the point of other tutorials. It's also dependent on the solver and the problem. So I can see a user getting confused why we use it for some things and not others.**

I think a compromise might be to drop some !!! tips around the place that they might consider solving the dual problem, rather than actually using it.

**Pointing them to a technical page on conic duality and the implementation of a specific solver is not an answer. Even the YALMIP docs, https://yalmip.github.io/command/dualize/, https://yalmip.github.io/tutorial/automaticdualization, point out that the user will likely be confused reading their documentation:

image

@blegat
Copy link
Member

blegat commented Jun 6, 2023

This is an extra for experts thing, and the addition of Dualization.dual_optimizer distracts from the point of other tutorials. It's also dependent on the solver and the problem. So I can see a user getting confused why we use it for some things and not others.

We shouldn't distract the user, we can just refer to the dualization tutorial saying that it's not specific to this tutorial so it's not a prerequisite for understanding the tutorial. It is however very important. We already had many users on discourse that were wondering why JuMP is slower than YALMIP. And that's only a fraction of users encountering this issue. Throwing users at conic formulation without mentioning that they should check dualization is like throwing someone in a fight without a sword. The argument should rather be whether we should mention dualization on tutorials that happen to be in the SCS format so we don't need it. Moreover, if we show the SCS log in the tutorial, the user will see that the dimensions are much larger than he would expect. When you read the documentation of SCS, it does not say in which format you should input it, dual or primal. We made an arbitrary decision when writing the MOI wrapper and the user cannot guess what this is. If we don't say anything, the user should expect that we dynamically choose the right one. But we don't, we just bridge to the format that we arbitrarily chose. Since we're not doing the right thing and leave to the user the job to dualize when needed we should at least let the user know about this responsibility. Because, intuitively, the user would think that we're doing it for him.

Even the YALMIP docs, https://yalmip.github.io/command/dualize/, https://yalmip.github.io/tutorial/automaticdualization, point out that the user will likely be confused reading their documentation:

I don't think this is what is meant. He is just referred to the previous 2 sentences that are purposely meant to be confusing (by using twice the words primal and dual) to emphasize that YALMIP is doing to nice things under the hood.
It is actually very simple. For performance reason, Dualization allows the solver to solve the dual form of the problem but it's all transparent so you don't have to understand more than that.
The same YALMIP doc page also says "this can lead to a much faster solution" and "This will however be very inefficient in YALMIP" so the whole page is rather advocating that the user should always check if this option makes the problem be solved faster.

@jd-foster
Copy link
Collaborator

See also #3408

@blegat
Copy link
Member

blegat commented Jun 6, 2023

See also #3408

Thanks! Another way to look at it. Here it looks like we give a lot of theoretical explanation on the dual, exemplified by the MAX-CUT only. and then for the conic example, we say we don't want to make the exercise and the user to figure it out by himself. If instead we say: in all conic examples, we are always dualizing if it's purely in one of the two forms then at least it gives the user more examples to practice his understanding. Otherwise it's like giving an exam directly after a lecture without a TA session ^^ In quantum_discrimination.jl we're like "try to figure it out, we know which form it is but we won't tell you". The user might also think we are not telling because it's so obvious so if it's not for him, he might think JuMP is not for him

@odow
Copy link
Member Author

odow commented Jun 6, 2023

Copying your comment from: #3399 (comment)

In view of how important it can be for a conic problem to solve well, it shouldn't be an option for expert though. Everyone conic user should use it and we should try to educate users to do so

Ah! So this is our source of disagreement then. If this is the case, then yes, it should be in every conic tutorial. And we should run examples with both SCS and Clarabel (for example) to show the difference. And it isn't sufficient just to sprinkle dual_optimizer in a few places.

But stepping back, if it's that important, shouldn't we choose to dualize automatically? (I think cvxpy does this?) This feels like the wrong decision to force users to learn.

@odow
Copy link
Member Author

odow commented Jun 6, 2023

we're like "try to figure it out, we know which form it is but we won't tell you". The user might also think we are not telling because it's so obvious so if it's not for him, he might think JuMP is not for him

But it is 'try it out' because if the use SCS then they should, but if they use a different solver then maybe they shouldn't. And we don't have good documentation for each solver on whether it expects the standard or geometric forms so there's no way for the average user to tell.

@blegat
Copy link
Member

blegat commented Jun 6, 2023

But stepping back, if it's that important, shouldn't we choose to dualize automatically? (I think cvxpy does this?) This feels like the wrong decision to force users to learn.

We probably should but unless it's pure standard or pure geometric, we don't really know which form to use. One idea would be to see the total sum of bridge cost for dualized and non-dualized version and choose that one. Then should be multiply the bridge cost by the number of constraints of that type ? So we could have an automatic mode doing this and then a manual mode where the user sets whether he want it dualized or not.

Even if we have that in the future, we should mitigate this issue in the meantime.

But it is 'try it out' because if the use SCS then they should, but if they use a different solver then maybe they shouldn't. And we don't have good documentation for each solver on whether it expects the standard or geometric forms so there's no way for the average user to tell.

Yes, it is "try it out". The message is "always try both", I think that's what your tutorial conveys when you say that if they plan to solve it multiple times or for a large instance then first try both and then choose and then run the large computational workload. Understanding why one is better is for the curious users but they don't need to understand why (if there was a need to understand why something works then ML/AI wouldn't be a thing :D)

@odow odow force-pushed the od/dualization branch from ca6a6b5 to fdc4425 Compare June 6, 2023 23:06
Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

LGTM once the missing word is added

@odow odow merged commit 85a68de into master Jun 7, 2023
@odow odow deleted the od/dualization branch June 7, 2023 22:25
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.

3 participants