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

Introducing Env #443

Merged
merged 4 commits into from
Feb 4, 2021
Merged

Introducing Env #443

merged 4 commits into from
Feb 4, 2021

Conversation

guimarqu
Copy link
Contributor

@guimarqu guimarqu commented Feb 3, 2021

  • mandatory argument for the algorithms
  • remove all global mutable variables (!)

In the future, Env will allows us to simplify algorithm architecture by moving storage in it

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #443 (c89bb81) into master (70faf97) will increase coverage by 0.00%.
The diff coverage is 92.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   80.13%   80.14%           
=======================================
  Files          52       50    -2     
  Lines        4299     4301    +2     
=======================================
+ Hits         3445     3447    +2     
  Misses        854      854           
Impacted Files Coverage Δ
src/Algorithm/basic/solvelpform.jl 93.87% <ø> (ø)
src/Algorithm/branching/branchingrule.jl 20.00% <ø> (ø)
src/Algorithm/branching/varbranching.jl 97.22% <ø> (ø)
src/Algorithm/divide.jl 54.54% <0.00%> (ø)
src/Algorithm/interface.jl 88.57% <0.00%> (ø)
src/Algorithm/preprocessing.jl 75.44% <ø> (ø)
src/MOIcallbacks.jl 93.61% <ø> (ø)
src/MathProg/MathProg.jl 100.00% <ø> (ø)
src/MathProg/clone.jl 100.00% <ø> (ø)
src/MathProg/vcids.jl 53.57% <ø> (ø)
... and 13 more

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 70faf97...9e85194. Read the comment docs.

@@ -51,14 +51,12 @@ end

getoptstate(data::BendersCutGenRuntimeData) = data.optstate

function run!(algo::BendersCutGeneration, rfdata::ReformData, input::OptimizationInput)::OptimizationOutput

function run!(algo::BendersCutGeneration, env::Env, rfdata::ReformData, input::OptimizationInput)::OptimizationOutput
Copy link
Collaborator

Choose a reason for hiding this comment

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

big line?

elapsed_time = @elapsed begin
optresult = TO.@timeit Coluna._to "relaxed master" run!(SolveLpForm(get_dual_solution = true), ModelData(master), OptimizationInput(OptimizationState(master)))
optresult = TO.@timeit Coluna._to "relaxed master" run!(SolveLpForm(get_dual_solution = true), env, ModelData(master), OptimizationInput(OptimizationState(master)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

big line?

@@ -721,7 +725,10 @@ function cg_main_loop!(
end
end

print_colgen_statistics(phase, iteration, stabstorage.curalpha, cg_optstate, nb_new_columns, rm_time, sp_time)
print_colgen_statistics(
env, phase, iteration, stabstorage.curalpha, cg_optstate, nb_new_columns,
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing whitespace?

sp_time += @elapsed begin
nb_new_col = solve_sps_to_gencols!(spinfos, algo, phase, data, redcostsvec, lp_dual_sol, smooth_dual_sol)
nb_new_col = solve_sps_to_gencols!(
spinfos, algo, env, phase, data, redcostsvec, lp_dual_sol,
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing whitespace?

Comment on lines +48 to +49
node::Node, algo::AbstractConquerAlgorithm, env::Env, data::ReformData,
storages_to_restore::StoragesUsageDict, opt_rtol::Float64 = Coluna.DEF_OPTIMALITY_RTOL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing whitespace?

)
!haskey(annotations.constrs_per_ann, mast_ann) && return
constrs = annotations.constrs_per_ann[mast_ann]
for (id, constr) in constrs
cloneconstr!(
origform, masterform, masterform, constr, MasterMixedConstr, loc_art_var = true
origform, masterform, masterform, constr, MasterMixedConstr,
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing whitespace?

annotations::Annotations,
mast_ann
mast_ann,
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra comma

@@ -136,7 +139,8 @@ function create_side_vars_constrs!(
name = string("sp_lb_", spuid)
lb_conv_constr = setconstr!(
masterform, name, MasterConvexityConstr;
rhs = lb_mult, kind = Essential, sense = Greater, inc_val = 100.0, loc_art_var = true
rhs = lb_mult, kind = Essential, sense = Greater, inc_val = 100.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing whitespace?

@@ -145,16 +149,17 @@ function create_side_vars_constrs!(
name = string("sp_ub_", spuid)
ub_conv_constr = setconstr!(
masterform, name, MasterConvexityConstr; rhs = ub_mult,
kind = Essential, sense = Less, inc_val = 100.0, loc_art_var = true
kind = Essential, sense = Less, inc_val = 100.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing whitespace?

prob::Problem, annotations::Annotations, reform::Reformulation, parent,
node::BD.Root
prob::Problem, reform::Reformulation, env::Env, annotations::Annotations, parent,
node::BD.Root,
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra comma

@rrsadykov
Copy link
Collaborator

I do not get the point of introducing environment. Is it just to store parameters? Storages are not global, they "belong" to a model, i.e. to the master, to a subproblem.

We need to create a document explaining the architecture of Coluna so that the changes to architecture are clear. Where is a place for such a document?

For the moment, we have models and algorithms. A model contain user data (formulation, etc) and computed data (storage). Algorithms are immutable and contain just the parameters, and they know child algorithms. I think it is good to have parameters local to algorithms. A user can pass either global parameters, which then are "localized" before solving (high-level usage). A user can also "fine-tune" local parameters of every algorithm, as it is done now (low-level usage). What is the advantage of having global parameters in an environment?

@guimarqu
Copy link
Contributor Author

guimarqu commented Feb 3, 2021

We need to create a document explaining the architecture of Coluna so that the changes to architecture are clear. Where is a place for such a document?

I'm writing the document, I'll send you it when I finish the first version.
I don't plan to change storages/algorithm architecture before.

What is the advantage of having global parameters in an environment?

The main goal is to remove all global mutable variables such as _params_ or _globals_.

@guimarqu guimarqu requested a review from vitornesello February 3, 2021 12:14
@guimarqu guimarqu merged commit 22537e5 into master Feb 4, 2021
@guimarqu guimarqu deleted the gm/env branch February 4, 2021 16:47
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.

3 participants