Vector of optimizers in `Formulation` #534

guimarqu · 2021-05-23T13:57:43Z

~~- needs Manifest because it uses master branch of BlockDecomposition~~

Design changes :

Formulation has a vector of optimizers (MoiOptimizer, PricingCallback or CustomOptimizer later)
~~run! methods of SolveIpForm, SolveLpForm, and PricingCallback receive a new optional argument optimizer_id which is the index of the optimizers attached to the Formulation.~~ See Vector of optimizers in Formulation #534 (comment)
methods to synchronize formulation with MoiOptimizers receive the optimizer as argument

rrsadykov · 2021-05-23T16:42:33Z

I do not like having an optional argument in the run!() method. If we change the algorithm for another optimization algorithm without this argument in the run!() method, we would have en error. The purpose of algorithms to be interchangeable. If we cannot change an algorithm for another, it is not an algorithm anymore.

Why not put optimizer_id directly to the definition of SolveIpForm, SolveLpForm, and PricingCallback? This would simplify a lot of things, and with this we would avoid adding arguments to the run!() method.

Also, the purpose of DefaultPricing algorithm is not clear anymore. The purpose of this algorithm was to switch pricing algorithms in the runtime. As now the user needs to set optimizer_id, we do not need to switch algorithms in the runtime. Why not setting directly SolveIpForm or PricingCallback with right optimizer_id as pricing_prob_solve_alg of ColumnGeneration?

guimarqu · 2021-05-23T20:23:49Z

I do not like having an optional argument in the run!() method. If we change the algorithm for another optimization algorithm without this argument in the run!() method, we would have en error. The purpose of algorithms to be interchangeable. If we cannot change an algorithm for another, it is not an algorithm anymore.

Since the argument is optional, you'll not have an error but you always have to use the first optimizer which is for me a major drawback. I tried to follow what I said in the discussion in your PR and to be quite conservative before having discussion.

Why not put optimizer_id directly to the definition of SolveIpForm, SolveLpForm, and PricingCallback? This would simplify a lot of things, and with this we would avoid adding arguments to the run!() method.

Also, the purpose of DefaultPricing algorithm is not clear anymore. The purpose of this algorithm was to switch pricing algorithms in the runtime. As now the user needs to set optimizer_id, we do not need to switch algorithms in the runtime. Why not setting directly SolveIpForm or PricingCallback with right optimizer_id as pricing_prob_solve_alg of ColumnGeneration?

These are the two options I saw this afternoon when working on this PR :

if we remove DefaultPricing, keep SolveIpForm SolveLpForm PricingCallback & put optimizer_id as parameter of these algorithms, then Coluna will throw an error when the user chooses SolveLpForm to call a UserOptimizer. I think we should avoid this by automatically detecting if the solver is a MoiOptimizer or a UserOptimizer (that's the job of DefaultPricing, it should be renamed by the way).
if we keep SolveIpForm SolveLpForm PricingCallback, put optimizer_id as parameter of these algorithms & letDefaultPricing do the dispatch depending on the type of the optimizer, then we have to make the types SolveIpForm, SolveLpForm,PricingCallback mutable to allow DefaultPricing to change their optimizer_id parameter before calling them. I don't know if it's ok to have mutable parameters for the algorithms.

What do you think ?

rrsadykov · 2021-05-24T08:07:58Z

Since the argument is optional, you'll not have an error but you always have to use the first optimizer which is for me a major drawback. I tried to follow what I said in the discussion in your PR and to be quite conservative before having discussion.

If you have an optional argument, you will use it at least once. In the call when this argument is used, you cannot use another algorithm which does not have this argument in the run!() function, you will get an error. Therefore, these two algorithms, one which calls and another which is called, are not independent anymore. Thus, you need to combine these algorithms together (or you should introduce another algorithm type, and add this argument to the input).

Coluna will throw an error when the user chooses SolveLpForm to call a UserOptimizer

I do not see a problem here. If parameterisation is inconsistent, there should be an error, it is normal for me. We cannot expect the user to always give a consistent parameterisation, especially if we mix paramerization in the algorithms and through BlockDecomposition.

I propose to put optimizer_id as a parameter in the algorithms with default value 1. We can leave DefaultPricing algorithm and rename it if you want. It will call the callback if the optimizer of formulation with id equal to optimizer_id of CallbackAlgorithm is of type UserOptimizer, otherwise it will call SolveIpForm. With this, the behaviour would be the same as before if the user gives only one solver for the subproblem. If no solver is set, then Coluna may automatically set the MOI default optimizer. However, if two or more solves are set, then user needs to use stages in column generation, and this requires a proper parameterisation, which may trigger errors if not properly set.

rrsadykov · 2021-05-24T08:08:27Z

I don't know if it's ok to have mutable parameters for the algorithms.

We should avoid that of course

rrsadykov · 2021-05-25T16:25:49Z

Ok, I like the current version better.

src/Algorithm/colgen.jl

rrsadykov · 2021-05-25T16:33:59Z

test/pricing_callback_tests.jl

-                                    pricing_prob_solve_alg = ClA.DefaultPricing(
-                                        optimizer_id=2, pricing_callback = PricingCallback(stage=1) 
+                                    pricing_prob_solve_alg = ClA.SolveIpForm(
+                                        optimizer_id=2, user_params = ClA.UserOptimize(stage=1) 


If user may define several solvers, he can define several callbacks (one heuristic and second exact). This way we can avoid stage parameter: user will just define optimizer_id=2 and optimizer_id=3. @guimarqu, what do you think about this?

I thought about that but forgot. So it's ok for me.

I open a new issue to adapt this

guimarqu · 2021-05-25T16:54:53Z

Ok, I like the current version better.

I was planning to write a comment to explain the changes after my break.

MoiOptimize and UserOptimize are not classic algorithm anymore although you can use them as algorithm. The user should rather use SolveIpForm that will dispatch depending on the type of the optimizer. It's kind of halfway between the implementation of SolveIpForm in master & what you did in the previous PR.

I'm going to fix your comments.

src/MathProg/optimizerwrappers.jl

vitornesello · 2021-05-25T19:15:22Z

src/Algorithm/basic/solveipform.jl

+    opt = getoptimizer(form, algo.optimizer_id)
+    params = _optimizer_params(algo, opt)
+    if params !== nothing
+        return run!(params, env, form, input; optimizer_id = algo.optimizer_id)
+    end


I think that there is some dispatch happening in runtime on lines 86, 87 and 89.
I think we should open an issue to investigate the impact later

Co-authored-by: Vitor Nesello <[email protected]>

rrsadykov · 2021-05-26T07:30:16Z

Ok for me, but I think the tests should pass before approving.

guimarqu · 2021-05-26T08:48:12Z

Tests pass on my machine. These changes need BlockDecomposition#master & when I push the Manifest, it breaks the CI. I don't want to release a new version of BlockDecomposition before having custom model/solver.

* dev branch to prepare release of 0.4.0 * Move storage & records in ColunaBase (#507) * Renaming in storage (#509) * RecordContainer -> RecordWrapper * state -> record * rename lot of things, remove getters of Storage not used by Algorithms * Storage -> StorageUnitWrapper * Deletion of `AbstractData` (#510) * RecordContainer -> RecordWrapper * state -> record * rename lot of things, remove getters of Storage not used by Algorithms * start removing AbstractData structs * tests ok * getunit -> getstorageunit; StorageDict -> Storage * fix docstring * Bijection StorageUnit -> Record (#518) * Bijection StorageUnit -> Record * address Ruslan's comments * Implementation of column generation stages (#525) * Implementation of column generation stages (for example, heuristic and exact stage) * Update after conversation with Guillaume + stabilization correction * Simplification for ColCutGenConquer * Some more modifs due to Guillaume comments * Counting the number of exact calls when testing the pricing stages (#530) * Add bound callback tests (#532) * add bound callback tests * include bound callback in runtests * fix test * Apply suggestions from code review Co-authored-by: Guillaume Marques <[email protected]> * add comment * Apply suggestions from code review Co-authored-by: Guillaume Marques <[email protected]> Co-authored-by: Guillaume Marques <[email protected]> * Vector of optimizers in `Formulation` (#534) * vector of optimizers in formulation * solver_id -> optimizer_id * add Manifest * update Manifest * remove Manifest because does not work * changes * rm files * address Ruslan's comment * Update src/MathProg/optimizerwrappers.jl Co-authored-by: Vitor Nesello <[email protected]> * add Manifest * change ci * remove ci change * rm Manifest Co-authored-by: Vitor Nesello <[email protected]> * UnitsUsageDict -> UnitsUsage (#522) * UnitsUsageDict -> UnitsAccess * wip * improve * tests ok * Custom data for variables and constraints (#495) * draft for support of customer data * custom data in solution * custom data for cut callback * computecoeff * store custom data of solutions in manager * add Manifest * rm Manifest * Support to custom cuts over custom data assigned to columns with new test * tests ok Co-authored-by: Artur Alves Pessoa <[email protected]> * Prototyping custom model/optimizer (#535) * Start example * wip draft * continue * add map * works with caching optimizer * varids in Env * wrong result * multiply costs by -1 * fix scaling * Apply suggestions from code review * Update test/interfaces/model.jl Co-authored-by: Lara Pontes <[email protected]> * Follow up of "custom data" (#538) * add AbstractCustomData and set/get inc_val * fix bugs * remove duplicate methods * delete unnecessary prefixes and fix some bugs * revert some changes and update docstring * custom information for dw sp (#542) * docstring for restricted master heuristic (#543) * docstring for restricted master heuristic * Update src/Algorithm/conquer.jl * Refactoring `ObjValues` & `OptimizationState` (#544) * clean + doc * move doc from objvalues to optstate; tests of objvalues; update ci * update * update branching priorioty deprecated method * tests ok * tests ok * tests ok * delete duplicated tests Co-authored-by: Ruslan Sadykov <[email protected]> Co-authored-by: Artur Pessoa <[email protected]> Co-authored-by: Lara di Cavalcanti Pontes <[email protected]> Co-authored-by: Vitor Nesello <[email protected]> Co-authored-by: Lara Pontes <[email protected]>

guimarqu added 5 commits May 23, 2021 15:57

vector of optimizers in formulation

00642d5

solver_id -> optimizer_id

1448f96

add Manifest

9efcd7b

update Manifest

f0b378d

remove Manifest because does not work

5b51b5b

guimarqu requested review from laradicp and vitornesello and removed request for laradicp May 23, 2021 14:11

changes

423149a

rrsadykov reviewed May 25, 2021

View reviewed changes

src/Algorithm/colgen.jl Outdated Show resolved Hide resolved

rrsadykov reviewed May 25, 2021

View reviewed changes

rm files

e98ef15

address Ruslan's comment

288e891

guimarqu mentioned this pull request May 25, 2021

Possibility to return primal and dual bounds in the pricing callback #527

Closed

vitornesello approved these changes May 25, 2021

View reviewed changes

Update src/MathProg/optimizerwrappers.jl

38e5c68

Co-authored-by: Vitor Nesello <[email protected]>

guimarqu requested a review from rrsadykov May 25, 2021 19:49

guimarqu added 4 commits May 26, 2021 10:32

add Manifest

a1feee2

change ci

b2604b2

remove ci change

27eaea7

rm Manifest

eb25e96

rrsadykov approved these changes May 26, 2021

View reviewed changes

guimarqu merged commit 69544da into release-0.4.0 May 26, 2021

guimarqu deleted the optimizer branch May 26, 2021 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vector of optimizers in `Formulation` #534

Vector of optimizers in `Formulation` #534

guimarqu commented May 23, 2021 •

edited

Loading

rrsadykov commented May 23, 2021

guimarqu commented May 23, 2021 •

edited

Loading

rrsadykov commented May 24, 2021 •

edited

Loading

rrsadykov commented May 24, 2021

rrsadykov commented May 25, 2021

rrsadykov May 25, 2021

guimarqu May 25, 2021

guimarqu May 25, 2021

guimarqu commented May 25, 2021 •

edited

Loading

vitornesello May 25, 2021

rrsadykov commented May 26, 2021

guimarqu commented May 26, 2021

Vector of optimizers in Formulation #534

Vector of optimizers in Formulation #534

Conversation

guimarqu commented May 23, 2021 • edited Loading

rrsadykov commented May 23, 2021

guimarqu commented May 23, 2021 • edited Loading

rrsadykov commented May 24, 2021 • edited Loading

rrsadykov commented May 24, 2021

rrsadykov commented May 25, 2021

rrsadykov May 25, 2021

Choose a reason for hiding this comment

guimarqu May 25, 2021

Choose a reason for hiding this comment

guimarqu May 25, 2021

Choose a reason for hiding this comment

guimarqu commented May 25, 2021 • edited Loading

vitornesello May 25, 2021

Choose a reason for hiding this comment

rrsadykov commented May 26, 2021

guimarqu commented May 26, 2021

Vector of optimizers in `Formulation` #534

Vector of optimizers in `Formulation` #534

guimarqu commented May 23, 2021 •

edited

Loading

guimarqu commented May 23, 2021 •

edited

Loading

rrsadykov commented May 24, 2021 •

edited

Loading

guimarqu commented May 25, 2021 •

edited

Loading