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

Fixed a bug when restoring master cuts in strong branching #444

Merged
merged 3 commits into from
Feb 11, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Algorithm/basic/cutcallback.jl
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ end

# CutCallbacks does not have child algorithms, therefore get_child_algorithms() is not defined

function get_storages_usage(algo::CutCallbacks, form::Formulation{MathProg.AbstractMasterDuty})
function get_storages_usage(algo::CutCallbacks, form::Formulation{Duty}
) where {Duty<:MathProg.AbstractFormDuty}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Sorry for this bug.

return [(form, MasterCutsStoragePair, READ_AND_WRITE)]
end

Expand Down
5 changes: 5 additions & 0 deletions src/Algorithm/basic/solvelpform.jl
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ function optimize_lp_form!(
algo::SolveLpForm, optimizer::MoiOptimizer, form::Formulation, result::OptimizationState
)
MOI.set(form.optimizer.inner, MOI.Silent(), algo.silent)
# MOI.set(form.optimizer.inner, MOI.RawParameter("Method"), 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove these comments ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem.

optimize_with_moi!(optimizer, form, result)
#Coluna.inc_lp_file_count()
#lpfile = "coluna_master$(Coluna.lp_file_count).lp"
#println("Writing LP file $lpfile")
#Gurobi.GRBwrite(form.optimizer.inner, lpfile)
return
end

Expand Down
4 changes: 0 additions & 4 deletions src/Algorithm/branching/branchingalgo.jl
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,6 @@ function perform_strong_branching_with_phases!(

sort!(groups, rev = true, by = x -> (x.isconquered, x.score))

if groups[1].isconquered
nb_candidates_for_next_phase == 1
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Artur, why did you remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Ruslan. I think it had no effect. Did you intend to use = instead of == ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyway, we decided to let the nodes survive a little bit more before being deleted to let them appear in the branch tree file. We don't think there will be a significant impact in the performance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it should be =. This is needed to avoid unnecessary evaluation of branching candidates, when one of candidates is already conquered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suggest marking it as conquered and testing it in the beginning of the evaluation to avoid evaluating it again but letting it persist until being written to the branch tree file (equivalent to BaPTree.dot). Otherwise some nodes will appear with only one child.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not completely get your point. Branching candidate (group) here is not a child node, but a complete set of child nodes (left branch and right branch). The group is already marked as conquered and will not be evaluated again. This is to avoid passing other candidates (or groups) to the next strong branching phase. Thus we avoid evaluating them. This evaluation is not needed as there exists a conquered branching candidate, i.e. all children nodes are conquered. We pass only this branching candidate, and all child nodes of this candidate will appear in the branch tree file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! ok. Got it. So, we should put the code back with = replacing ==.


# before deleting branching groups which are not kept for the next phase
# we need to remove storage states kept in these nodes
for group_index = nb_candidates_for_next_phase + 1 : length(groups)
Expand Down
7 changes: 7 additions & 0 deletions src/Algorithm/storage.jl
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ const StorageTypePair = Pair{DataType, DataType}
# be restored for writing (all other storages are restored anyway but just for reading)
const StoragesUsageDict = Dict{Tuple{AbstractModel, StorageTypePair}, StorageAccessMode}

function Base.show(io::IO, stoUsaDict::StoragesUsageDict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable name must be lower case.

print(io, "storage usage dict [")
for usage in stoUsaDict
print(io, " (", typeof(usage[1][1]), ", ", usage[1][2], ") => ", usage[2])
end
print(io, " ]")
end

"""
function add_storage_pair_usage!(::StoragesUsageDict, ::AbstractModel, ::StorageTypePair, ::StorageAccessMode)
Expand Down
2 changes: 1 addition & 1 deletion src/Algorithm/treesearch.jl
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ function print_node_in_branching_tree_file(
ncur = get_tree_order(node)
time = elapsed_optim_time(env)
if ip_gap_closed(getoptstate(node))
@printf file "\n\tn%i [label= \"N_%i (%.0f s) \\nPRUNED\"];" ncur ncur time
@printf file "\n\tn%i [label= \"N_%i (%.0f s) \\n[PRUNED , %.4f]\"];" ncur ncur time pb
else
@printf file "\n\tn%i [label= \"N_%i (%.0f s) \\n[%.4f , %.4f]\"];" ncur ncur time db pb
end
Expand Down
34 changes: 34 additions & 0 deletions src/Coluna.jl
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,40 @@ const TOL = 1e-8 # if - ϵ_tol < val < ϵ_tol, we consider val = 0
const TOL_DIGITS = 8 # because round(val, digits = n) where n is from 1e-n
###

### Debugging tools
global debug_count = 0
function inc_debug_count()
global debug_count += 1
return
end
function debug_stack()
for l in stacktrace()
println(l)
end
end
macro debug()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add a debug parameter and a test to cover these debug functions.

Copy link
Contributor

@guimarqu guimarqu Feb 8, 2021

Choose a reason for hiding this comment

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

The debug tool should not stay in the code. Can we move it to the documentation ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem. I just did not want to loose it.

return esc(:(
cont = false;
while !cont
print("\njulia> ");
cmd = readline();
if cmd == ""
cont = true;
else
try
eval(Meta.parse(cmd));
catch err
println(err);
end
end
end
))
end
###

# submodules
export ColunaBase, MathProg, Algorithm

const _to = TO.TimerOutput()

export Algorithm, ColunaBase, MathProg, Env, DefaultOptimizer, Parameters,
Expand Down
2 changes: 1 addition & 1 deletion src/MOIcallbacks.jl
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function MOI.submit(
end

constr = setconstr!(
form, "", MasterMixedConstr;
form, "", MasterBendCutConstr; #TODO: Create a specific type for user cuts
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the duty ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is important. I had to fix two things to make it work. This is one of them. The user cuts were not being recognized by the constructor of MasterCutsState because of the condition "getduty(id) <= AbstractMasterCutConstr" and I did not want to change the complex hierarchy defined in "duties.jl". So defining it as a MasterBendCutConstr was a solution with minimum changes to avoid generating new bugs.

rhs = rhs,
kind = Essential,
sense = sense,
Expand Down