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

fix isinteger + continuous columns if sp has only continuous vars #551

Merged
merged 2 commits into from
Jul 5, 2021

Conversation

guimarqu
Copy link
Contributor

@guimarqu guimarqu commented Jul 3, 2021

fix part of #550

@guimarqu guimarqu requested a review from rrsadykov July 3, 2021 13:30
@codecov
Copy link

codecov bot commented Jul 3, 2021

Codecov Report

Merging #551 (0bc7144) into master (3ef9f73) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #551   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files          47       47           
  Lines        4685     4685           
=======================================
  Hits         4036     4036           
  Misses        649      649           
Impacted Files Coverage Δ
src/MathProg/duties.jl 63.63% <ø> (ø)
src/Algorithm/colgen.jl 95.72% <100.00%> (-0.03%) ⬇️
src/MathProg/formulation.jl 77.87% <100.00%> (-0.27%) ⬇️
src/MathProg/solutions.jl 66.66% <100.00%> (+2.38%) ⬆️
src/decomposition.jl 99.15% <100.00%> (+0.02%) ⬆️
src/MathProg/varconstr.jl 87.89% <0.00%> (-0.64%) ⬇️

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 3ef9f73...0bc7144. Read the comment docs.

@guimarqu guimarqu marked this pull request as draft July 3, 2021 13:57
@guimarqu guimarqu marked this pull request as ready for review July 3, 2021 14:06
@rrsadykov
Copy link
Collaborator

rrsadykov commented Jul 3, 2021

Guillaume, determination of solution integrality is quite a complex issue. In principle, you should impose integrality constraints only on original variables, but not on columns. In general, columns should be continuous variables. As I remember, the only special case when you can declare columns as integer is when the corresponding subproblem has only binary variables. In other cases (some variables are general integer or continuous), columns should be continuous.

Also, in the restricted master heuristic, you should add subproblem representative variables which are integer, even if they are implicit during column generation, and put integrality restrictions on them. Also the constraints linking subproblem representative variables with columns should be added. Again, the only special case when you can just put integrality constraints on columns is when all subproblem variables are binary.

Additional complication is to obtain disaggregated solution from a solution which is integer in original variables. In principle, when you verify whether a solution is integer, you should obtain this disaggregated solution. Only if you can do it, the solution is indeed integer. As I remember, one should look at the paper of François for a general (lexicographic) algorithm which obtains a disaggregated solution:

@article{Vanderbec:11a,
	author = {Vanderbeck, Fran\c{c}ois},
	journal = {Mathematical Programming},
	number = {2},
	pages = {249-294},
	title = {Branching in branch-and-price: a generic scheme},
	volume = {130},
	year = {2011}}

I think François can give a more precise reference.

@guimarqu
Copy link
Contributor Author

guimarqu commented Jul 3, 2021

Thanks for the feedback ! Yes I took a look at this paper last week after I saw the solution returned by the bin-packing (#547, you can see the disaggregated solution in the second edition of the main message by @laradicp )

The idea here was to set the perennial kind of master variables to integer if the subproblem has integer variables, and continuous otherwise.
The current kind of master columns is continuous throughout the branch-and-cut-and-price expect in the restricted master heuristic where we enforce integrality on master columns only if they have an integer perennial kind (because it doesn't make sense to enforce integrality on columns from a subproblem that has only continuous variables).

When I implemented this change, I indeed didn't realize that isinteger(disaggregated_solution) may miss lots of feasible solutions. However, at the moment, we are checking the integrality of the aggregated solutions only.

Besides checking the integrality of the aggregated solutions, we'll have to check the integrality of disaggregated solutions when the subproblem has upper multiplicity greater than 1. Ideally, we would need to implement François' algorithm, but in the meantime, we can just check if master variables with integer perennial kind have integer values (at the price of missing some feasible solutions).

It looks like the concept of "integer column" is something very specific that I'm not familiar with.

So, the main issue here is how to not enforce integrality of columns from subproblem that has no integer variables. You suggest to make all sp representative variables active with integrality only on those which are integer and create a constraint for each of these variables to link them with columns. Did I understand ? Is it good for performances ?

Is it worth it to keep the integer perennial kind for only the "integer column" case you described ? Is it good for performances ?

@rrsadykov
Copy link
Collaborator

I accept this merge request. This version should work ok for cases when subproblem variables are continuous or binary.

For the restricted master heuristic, the current behaviour is ok, as we are only restricting feasible solutions (we may miss some integer solutions in the case when subproblem variables are general integer but it is ok). My suggestion to add subproblem representative variables which are integer to the restricted master heuristic would improve efficiency of the restricted master heuristic in some cases, but this change is optional.

The current issue that in some cases a solution may be declared integer (because all original variables have integer values), but no disaggregated integer solution exist. This should be addressed in the future. As I understand, this may happen only if there are general integer subproblem variables and subproblems multiplicity is greater than one (although I am not 100% sure).

@guimarqu guimarqu merged commit ce77692 into master Jul 5, 2021
@guimarqu guimarqu deleted the continuous_cols branch July 5, 2021 11:18
@guimarqu guimarqu mentioned this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants