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

Alias for a simplified BCP parameterisation #603

Merged
merged 12 commits into from
Oct 4, 2021
Merged

Alias for a simplified BCP parameterisation #603

merged 12 commits into from
Oct 4, 2021

Conversation

rrsadykov
Copy link
Collaborator

Introduces Coluna.Algorithm.BranchCutAndPriceAlgorithm alias for a simplified parameterisation of the branch-cut-and-price algorithm.

Addresses issue #478.

@rrsadykov rrsadykov requested review from guimarqu, vitornesello, FD-V and artalvpes and removed request for vitornesello and FD-V September 13, 2021 15:50
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #603 (aefae7e) into master (5870353) will increase coverage by 0.06%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
+ Coverage   85.64%   85.70%   +0.06%     
==========================================
  Files          47       48       +1     
  Lines        4834     4870      +36     
==========================================
+ Hits         4140     4174      +34     
- Misses        694      696       +2     
Impacted Files Coverage Δ
src/Algorithm/branching/varbranching.jl 95.83% <90.00%> (-1.67%) ⬇️
src/Algorithm/branchcutprice.jl 100.00% <100.00%> (ø)
src/Algorithm/branching/branchingalgo.jl 88.97% <0.00%> (-0.71%) ⬇️

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 5870353...aefae7e. Read the comment docs.

@guimarqu
Copy link
Contributor

I'll review tomorrow

@guimarqu
Copy link
Contributor

guimarqu commented Sep 22, 2021

I think that before merging this new interface, we must decide on how we define the branching priorities.

My suggestion is to define branching priorities only through BlockDecomposition. Currently, we only have BlockDecomposition.branchingpriority! to define the branching priority on a JuMP variable. I think it can replace VarBranchingRule. However, if the latter disappears, AbstractBranchingRule and PrioritisedBranchingRule also disappear.

The aim of AbstractBranchingRule is not clear to me.

Edit: diagram class of strong branching

classDiagram
class PrioritisedBranchingRule {
    float root_priority
    float nonroot_priority
}
class BranchingPhase {
    int max_nb_candidates
    AbstractConquerAlgo conquer_algo
}
StrongBranching *-- PrioritisedBranchingRule
StrongBranching *-- BranchingPhase
PrioritisedBranchingRule *-- AbstractBranchingRule
AbstractBranchingRule <|-- VarBranchingRule

@rrsadykov
Copy link
Collaborator Author

rrsadykov commented Sep 23, 2021

AbstractBranchingRule is the algorithm type. Algorithms of this type find branching candidates. VarBranchingRule is a particular algorithm to perform branching. There can be other branching rules: Ryan&Foster, branching on constraints (defined by ``@expressionin JuMP), or the user can define his own branching rules. May be we should renameBranchingRule` to `BranchingAlgorithm` to be more clear.

The main problem here is that we mix variable branching priorities given using BlockDecomposition and branching algorithms defined in Coluna (these two parameterisations should be "compatible"). We cannot have just variable branching priorities, as it is important to be able to do other kinds of branching (non-robust branching is sometimes necessary for completeness).

One solution may be to define variable branching using names, like

branchingFirstOnXthenOnY = ClA.StrongBranching(
            phases = [] ,
            rules = [ClA.PrioritisedBranchingRule(ClA.VarBranchingRule(varname = "X"),  2.0, 2.0),
                         ClA.PrioritisedBranchingRule(ClA.VarBranchingRule(varname = "Y"), 1.0, 1.0)]
        )

For this, we need to be able to get the JuMP name of a variable (or expression) inside the branching algorithm in Coluna.

Then, branching priorities given using BlockDecomposition will serve just to sort individual variables with the same name. So, the "upper level" branching priorities will be given in the Coluna parameterisation, and "lower level" branching priorities will be given using BlockDecomposition. Actually, this is completely compatible with the current definition of ClA.VarBranchingRule(), which "covers" all variables independently of their name. We just need to slightly change its implementation: it should sort variables (and not filter them) according to their branching priorities.

One may also parameterise VarBranchingRule with the selection strategy (most violated, smallest index, etc). This is how it is done in constraint programming solvers.

@guimarqu
Copy link
Contributor

Ok, thanks for the explanation. I had doubts about the aim of AbstractBranchingRule because there is currently only one rule implemented & because the selection criterion is in StrongBranching.

About your suggestion, I don't think we can rely on MathOptInterface to always provide us the name of the variables and constraints. Currently, if you build your model using the caching optimizer (which is the default mode), Coluna does not receive the name of variables and constraints from MathOptInterface.
Moreover, users may want to use bridges to transform variables and constraints unsupported by Coluna into supported ones.
I guess that some bridges may create new variables but how do you propagate the branching rules to these new variables with your suggestion ? I think that these new variables have no name.

@rrsadykov
Copy link
Collaborator Author

rrsadykov commented Sep 23, 2021

Receiving names of variables (and constraints) is critical for branching. In many cases, the user needs to see on which variable the branching is performed. Otherwise we cannot position Coluna as useful for research purposes.

In the worst case the name should be passed through BlockDecomposition. One may add variable name argument in BlockDecomposition.branchingpriority!() or add another function BlockDecomposition.varname!().

@rrsadykov
Copy link
Collaborator Author

rrsadykov commented Sep 23, 2021

Moreover, users may want to use bridges to transform variables and constraints unsupported by Coluna into supported ones.
I guess that some bridges may create new variables but how do you propagate the branching rules to these new variables with your suggestion ? I think that these new variables have no name.

This is too advanced for me. Created variables may just receive some default (f.e. empty) name. The user then knows that branching is done on a variable not defined by him.

@guimarqu
Copy link
Contributor

guimarqu commented Sep 23, 2021

I agree that showing names of variables in branching is very useful when you implement a new application or in the R&D phase. However, I don't think we need them in production, so it's not so critical nor a requirement.

I guess created variables receive an empty name. However, I think we should be able to propagate the branching rule defined on the original variables because the latter may disappear. This propagation is done by MathOptInterface with a bridge that we will define for the BlockDecomposition.BranchingPriority attribute.
If we define branching rules in the algorithm, we can't do the propagation anymore.

I think it's very important to not try to bypass what has been done in MathOptInterface because we'll gain nothing except new bugs. The names should be provided by JuMP and MathOptInterface, not by BlockDecomposition.

@rrsadykov
Copy link
Collaborator Author

rrsadykov commented Sep 23, 2021

However, I don't think we need them in production, so it's not so critical nor a requirement.

In research, there is no production phase.

The names should be provided by JuMP and MathOptInterface, not by BlockDecomposition

I completely agree, but you said that we cannot count on that.

I general, I do not understand what do you suggest to do with branching.

@guimarqu
Copy link
Contributor

guimarqu commented Sep 28, 2021

I think we can define both branching priorities and branching rules through BlockDecomposition

branchingpriority!(x; rule = Coluna.VarBranchingRule(), root_priority = 1, non_root_priority = 2)

Before starting the optimization, Coluna can group variables that have the same branching rules. Names are not a requirement anymore.

Ruslan Sadykov and others added 5 commits October 1, 2021 11:41
- Modification of the VarBranchingRule algorithm : now all the fractional variables of the maximum branching priority (set in BlockDecomposition) are selected
src/Algorithm/branchcutprice.jl Outdated Show resolved Hide resolved
src/Algorithm/branchcutprice.jl Outdated Show resolved Hide resolved
src/Algorithm/branchcutprice.jl Outdated Show resolved Hide resolved
src/Algorithm/branchcutprice.jl Outdated Show resolved Hide resolved
src/Algorithm/branchcutprice.jl Outdated Show resolved Hide resolved
src/Algorithm/branching/varbranching.jl Outdated Show resolved Hide resolved
src/Algorithm/branching/varbranching.jl Outdated Show resolved Hide resolved
src/Algorithm/branching/varbranching.jl Outdated Show resolved Hide resolved
src/Algorithm/branching/varbranching.jl Outdated Show resolved Hide resolved
brpriority = getbranchingpriority(master, var_id)
if (brpriority > max_priority)
max_priority = brpriority
selected_vars = [(var_id, val)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean that you create a new array each time you find a non-integer variable that has a greater priority than the current greatest priority?

I personally think it's better to loop twice over the solution. A first time to get the greatest branching priority, a second time to fill the groups array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I have modified the code in the latest commit as you suggested

@rrsadykov rrsadykov requested a review from guimarqu October 4, 2021 10:26
@guimarqu guimarqu merged commit ee59439 into master Oct 4, 2021
@guimarqu guimarqu deleted the bcp_alias branch October 4, 2021 10:44
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