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

artalvpes · 2021-02-06T21:03:49Z

Note: I have added several (disabled) debugging tools used to find the bug.

…eral debugging tools used to find the bug

codecov · 2021-02-06T21:10:43Z

Codecov Report

Merging #444 (e573dc3) into master (22537e5) will increase coverage by 0.41%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   80.14%   80.55%   +0.41%     
==========================================
  Files          50       50              
  Lines        4301     4305       +4     
==========================================
+ Hits         3447     3468      +21     
+ Misses        854      837      -17

Impacted Files	Coverage Δ
src/Algorithm/basic/cutcallback.jl	`96.29% <ø> (+7.01%)`	⬆️
src/Algorithm/branching/branchingalgo.jl	`91.59% <0.00%> (ø)`
src/Algorithm/storage.jl	`74.59% <0.00%> (-2.34%)`	⬇️
src/Coluna.jl	`100.00% <ø> (ø)`
src/MOIcallbacks.jl	`93.61% <ø> (ø)`
src/MathProg/duties.jl	`40.00% <ø> (ø)`
src/Algorithm/treesearch.jl	`91.39% <100.00%> (ø)`
src/Algorithm/formstorages.jl	`51.25% <0.00%> (+11.87%)`	⬆️

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 22537e5...f250ab3. Read the comment docs.

rrsadykov

Artur, can you explain how your debugging tools work?

rrsadykov · 2021-02-08T08:20:14Z

src/Algorithm/branching/branchingalgo.jl

@@ -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


Artur, why did you remove this?

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

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.

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

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.

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.

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

rrsadykov · 2021-02-08T08:20:45Z

src/Algorithm/basic/cutcallback.jl

@@ -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} 


Nice! Sorry for this bug.

rrsadykov · 2021-02-08T08:22:41Z

src/Coluna.jl

+        println(l)
+    end
+end
+macro debug()


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

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

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

guimarqu · 2021-02-08T08:46:51Z

src/Algorithm/basic/solvelpform.jl

@@ -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)


Can we remove these comments ?

No problem.

guimarqu · 2021-02-08T08:48:21Z

src/MOIcallbacks.jl

@@ -100,7 +100,7 @@ function MOI.submit(
    end

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


Why did you change the duty ?

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.

artalvpes · 2021-02-09T12:35:01Z

I think it is worth letting a general comment about the code structure here. I think it would help a lot for people that starts looking at the code if there was a more clearly marked standard structure for the storages that were followed by all algorithms. Perhaps we could put each algorithm and each storage type in a separate file and force all storage functions to be redefined (like get_storages_usage and get_child_algorithms) even if they do nothing (by generating an error message in the function that receives the abstract type). Calls to restore_states!, store_states! and remove_states! could perhaps be more emphasised and/or we could try to keep them in the first call level (function run!) of their algorithms.

rrsadykov · 2021-02-09T13:22:12Z

I agree that we need a document describing the architecture of Coluna (environment, models, formulations, algorithms, storages, and other concepts we use). Guillaume should have started such a document. I will review and complete it when Guillaume is done with his part. A link to this document should be in the README.md file.

…nches, and creating a new duty for master cuts

guimarqu

One style issue to address and then the PR is ready to be merged.

guimarqu · 2021-02-10T18:51:51Z

src/Algorithm/storage.jl

@@ -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)


Variable name must be lower case.

Fixed a bug when restoring master cuts in strong branching. Added sev…

3ab7df6

…eral debugging tools used to find the bug

rrsadykov reviewed Feb 8, 2021

View reviewed changes

guimarqu reviewed Feb 8, 2021

View reviewed changes

Removing debug codes, stopping the branch evaluation on conquered bra…

2ae1f4f

…nches, and creating a new duty for master cuts

guimarqu suggested changes Feb 10, 2021

View reviewed changes

Correcting the style

f250ab3

guimarqu approved these changes Feb 11, 2021

View reviewed changes

guimarqu merged commit 03ef7dc into master Feb 11, 2021

guimarqu deleted the fix_strongbranching branch February 11, 2021 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

artalvpes commented Feb 6, 2021

codecov bot commented Feb 6, 2021 •

edited

Loading

rrsadykov left a comment

rrsadykov Feb 8, 2021

artalvpes Feb 9, 2021

artalvpes Feb 9, 2021

rrsadykov Feb 9, 2021

artalvpes Feb 9, 2021

rrsadykov Feb 9, 2021

artalvpes Feb 9, 2021

rrsadykov Feb 8, 2021

rrsadykov Feb 8, 2021

guimarqu Feb 8, 2021 •

edited

Loading

artalvpes Feb 9, 2021

guimarqu Feb 8, 2021

artalvpes Feb 9, 2021

guimarqu Feb 8, 2021

artalvpes Feb 9, 2021

artalvpes commented Feb 9, 2021

rrsadykov commented Feb 9, 2021

guimarqu left a comment

guimarqu Feb 10, 2021

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

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

Conversation

artalvpes commented Feb 6, 2021

codecov bot commented Feb 6, 2021 • edited Loading

Codecov Report

rrsadykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guimarqu Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artalvpes commented Feb 9, 2021

rrsadykov commented Feb 9, 2021

guimarqu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2021 •

edited

Loading

guimarqu Feb 8, 2021 •

edited

Loading