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] fix benders_decomposition.jl #3532

Merged
merged 6 commits into from
Oct 8, 2023
Merged

[docs] fix benders_decomposition.jl #3532

merged 6 commits into from
Oct 8, 2023

Conversation

odow
Copy link
Member

@odow odow commented Oct 4, 2023

Closes #3531

Thanks for catching this @WalterMadelim. I'm surprised that it didn't error!

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (df022a5) 98.25% compared to head (64017e5) 98.18%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3532      +/-   ##
==========================================
- Coverage   98.25%   98.18%   -0.08%     
==========================================
  Files          37       37              
  Lines        5563     5561       -2     
==========================================
- Hits         5466     5460       -6     
- Misses         97      101       +4     

see 4 files with indirect coverage changes

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

@@ -266,7 +266,7 @@ function my_callback(cb_data)
return
end
cut = @build_constraint(θ >= ret.obj + -ret.π' * A_1 * (x .- x_k))
MOI.submit(model, MOI.LazyConstraint(cb_data), cut)
MOI.submit(lazy_model, MOI.LazyConstraint(cb_data), cut)
Copy link
Member Author

Choose a reason for hiding this comment

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

I should look at why this didn't error. It should have...

Choose a reason for hiding this comment

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

image
It has indeed, after changing GLPK to Gurobi. (I have not installed GLPK)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so this might be an issue with GLPK, not with JuMP. Thanks for looking.

@odow
Copy link
Member Author

odow commented Oct 4, 2023

Now I don't understand what's going on here:
image

@odow
Copy link
Member Author

odow commented Oct 4, 2023

GLPK callbacks are weird

@WalterMadelim
Copy link

I think the output in your figure is normal. There are multiple "Terminating with the optimal solution", because this is a callback function, which will be called frequently during solving, and the solver's terminating criterion doesn't depend on user's callback function. As for the Jumping up - Jumping down lower bound, this is also reasonable because the problem is changing during solving. The puzzling point is why GLPK can return the right answer, even if you submit to model not lazy_model.

But it should be fine after revising. I agree with the new callback function.

@WalterMadelim
Copy link

GLPK callbacks are weird

Actually I would prefer to use a solver-dependent callback. (For example, I would refer to Gurobi.jl directly when I have a need)

@odow
Copy link
Member Author

odow commented Oct 6, 2023

@WalterMadelim
Copy link

I thought there might be no one who is more familiar with this than you, because it seems that you are the main contributer of GLPK.jl.🥲

From my point of view, the function my_callback(cb_data) is special, because it is controlled by the solver rather than Julia. I tried to put break points @bp's inside it, but they were never hitted. Therefore this function is more likely to be a component of the branch and cut module: when the main algorithm need a user's callback, then a lazy cut is retrieved and stored.

Therefore, I think the name number_of_subproblem_solves is not proper. It's the number of times that callback module in GLPK is executed when status == MOI.CALLBACK_NODE_STATUS_INTEGER. Subproblems modeling and branching should reside in a deeper module than callback module in GLPK (my comprehension).

This adapted version of TSP callback code (using MOI without JuMP) might be a comparison. Solver is Gurobi, the problem is larger and the counter reaches 20, compared with your result 341. (you can try it)

https://github.com/WalterMadelim/p8f.jl/blob/main/src/TSP_GRB_callback.jl

@odow
Copy link
Member Author

odow commented Oct 6, 2023

I thought there might be no one who is more familiar with this than you, because it seems that you are the main contributer of GLPK.jl

Yes 😄 I meant this as "given I understand exactly how it works, it is confusing that it is called so many times."

@odow odow merged commit 2368e63 into master Oct 8, 2023
9 of 11 checks passed
@odow odow deleted the odow-patch-1 branch October 8, 2023 09:57
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 problems found in doc Benders Decomposition
2 participants