Update Formulation docstring #484

laradicp · 2021-03-26T15:40:34Z

task for #486

codecov · 2021-03-26T15:45:42Z

Codecov Report

Merging #484 (55f28e2) into master (e5113ab) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
- Coverage   83.06%   82.98%   -0.08%     
==========================================
  Files          48       48              
  Lines        4700     4697       -3     
==========================================
- Hits         3904     3898       -6     
- Misses        796      799       +3

Impacted Files	Coverage Δ
src/Coluna.jl	`100.00% <100.00%> (ø)`
src/MOIwrapper.jl	`66.85% <100.00%> (-0.10%)`	⬇️
src/MathProg/formulation.jl	`73.70% <100.00%> (ø)`
src/MathProg/problem.jl	`85.18% <100.00%> (-0.53%)`	⬇️
src/decomposition.jl	`99.11% <100.00%> (ø)`
src/optimize.jl	`94.11% <100.00%> (-0.09%)`	⬇️
src/Algorithm/branching/branchinggroup.jl	`88.42% <0.00%> (-1.06%)`	⬇️
src/Algorithm/branching/branchingalgo.jl	`90.32% <0.00%> (-0.81%)`	⬇️
src/Algorithm/storage.jl	`76.11% <0.00%> (-0.75%)`	⬇️

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 e5113ab...55f28e2. Read the comment docs.

guimarqu · 2021-03-26T15:53:14Z

Do you think we can improve this method ?

Something like :

create_formulation!(problem::Problem, duty::Type{<:AbstractFormDuty}; 
     obj_sense = MinSense, parent_formulation = nothing)

laradicp · 2021-03-26T16:30:41Z

Do you think we can improve this method ?

Something like :

create_formulation!(problem::Problem, duty::Type{<:AbstractFormDuty}; 
     obj_sense = MinSense, parent_formulation = nothing)

Yes, that makes sense to me. Should the method be kept in "formulation.jl" even with the ::Problem parameter?

guimarqu · 2021-03-26T16:42:11Z

Do you think we can improve this method ?
Something like :
create_formulation!(problem::Problem, duty::Type{<:AbstractFormDuty}; 
     obj_sense = MinSense, parent_formulation = nothing)
Yes, that makes sense to me. Should the method be kept in "formulation.jl" even with the ::Problem parameter?

Good question because we'll have cross-dep issue . Besides I would like to move problem outside MathProg (cf #439) so it's definitely not a good idea.
EDIT : also confict with #268

It's also does not look good to have Reformulation as first parameter.

Do you think we can move the formulation counter into the environment and then have :

create_formulation!(env::Env, duty::Type{<:AbstractFormDuty}; 
     obj_sense = MinSense, parent_formulation = nothing)

?

It fits with the def of Env

Env is a place to store global information during the whole execution of Coluna. The environment contains the global parameter ...

laradicp · 2021-03-26T18:08:35Z

Do you think we can improve this method ?
Something like :
create_formulation!(problem::Problem, duty::Type{<:AbstractFormDuty}; 
     obj_sense = MinSense, parent_formulation = nothing)
Yes, that makes sense to me. Should the method be kept in "formulation.jl" even with the ::Problem parameter?
Good question because we'll have cross-dep issue . Besides I would like to move problem outside MathProg (cf #439) so it's definitely not a good idea.
EDIT : also confict with #268

It's also does not look good to have Reformulation as first parameter.

Do you think we can move the formulation counter into the environment and then have :
create_formulation!(env::Env, duty::Type{<:AbstractFormDuty}; 
     obj_sense = MinSense, parent_formulation = nothing)
?

It fits with the def of Env

Env is a place to store global information during the whole execution of Coluna. The environment contains the global parameter ...

I see, I tried to do that just now. Env is defined after including MathProg in "Coluna.jl". I tried to include it after Env definition, but ::Counter is from MathProg.

guimarqu · 2021-03-26T18:11:46Z

It's ok to move Counter into ColunaBase.

guimarqu · 2021-03-26T18:14:21Z

By the way, do we really need a Counter to maintain the number of formulations in the Coluna environment ? It looks like we did overengineering.

laradicp · 2021-03-27T05:38:09Z

By the way, do we really need a Counter to maintain the number of formulations in the Coluna environment ? It looks like we did overengineering.

Could the Counter be a way of passing the argument as a mutable value?

guimarqu · 2021-03-27T07:18:21Z

Env is a mutable struct, so I think you can replace the counter by creating a field nb_formulations of type Int in Env.

laradicp · 2021-03-27T14:59:38Z

Env is a mutable struct, so I think you can replace the counter by creating a field nb_formulations of type Int in Env.

Sorry, I meant in the getnewuid function, used in the Formulation constructor, the argument passed is only form_counter. If it is an Int, I think its value cannot be changed. But I can put the operation made in that function outside or pass Env instead of Env.form_counter as the argument.

guimarqu · 2021-03-27T15:07:37Z

Yes you can pass Env as argument. Guillaume Marques Sent from ProtonMail Le sam., mars 27, 2021 à 15:59, Lara di Cavalcanti Pontes ***@***.***> a écrit :

…

> Env is a mutable struct, so I think you can replace the counter by creating a field nb_formulations of type Int in Env. Sorry, I meant in the getnewuid function, used in the Formulation constructor, the argument passed is only form_counter. If it is an Int, I think its value cannot be changed. But I can put the operation made in that function outside or pass Env instead of Env.form_counter as the argument. — You are receiving this because your review was requested. Reply to this email directly, [view it on GitHub](#484 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABNM5B3KMJFEDUMDYZM6DUTTFXXGNANCNFSM4Z3T3PYA).

guimarqu

Hi,

You have to update the documentation (you can add the reference here : https://github.com/atoptima/Coluna.jl/blob/master/docs/src/dev/todo.md) to fix the error :

┌ Error: 1 docstring not included in the manual:
│ 
│     Coluna.MathProg.create_formulation! :: Tuple{Env, Type{var"#s2"} where var"#s2"<:Coluna.MathProg.AbstractFormDuty}
│ 
│ 
│ These are docstrings in the checked modules (configured with the modules keyword)
│ that are not included in @docs or @autodocs blocks.
└ @ Documenter.DocChecks ~/.julia/packages/Documenter/6vUwN/src/DocChecks.jl:69
[ Info: Populate: populating indices.
ERROR: LoadError: `makedocs` encountered an error. Terminating build
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] runner(#unused#::Type{Documenter.Builder.RenderDocument}, doc::Documenter.Documents.Document)
   @ Documenter.Builder ~/.julia/packages/Documenter/6vUwN/src/Builder.jl:255
 [3] dispatch(#unused#::Type{Documenter.Builder.DocumentPipeline}, x::Documenter.Documents.Document)
   @ Documenter.Utilities.Selectors ~/.julia/packages/Documenter/6vUwN/src/Utilities/Selectors.jl:170
 [4] #2
   @ ~/.julia/packages/Documenter/6vUwN/src/Documenter.jl:247 [inlined]
 [5] cd(f::Documenter.var"#2#3"{Documenter.Documents.Document}, dir::String)
   @ Base.Filesystem ./file.jl:106
 [6] #makedocs#1
   @ ~/.julia/packages/Documenter/6vUwN/src/Documenter.jl:246 [inlined]
 [7] top-level scope
   @ ~/work/Coluna.jl/Coluna.jl/docs/make.jl:3
in expression starting at /home/runner/work/Coluna.jl/Coluna.jl/docs/make.jl:3
Error: Process completed with exit code 1.

guimarqu · 2021-03-28T17:33:09Z

src/MathProg/formulation.jl

-Construct a `Formulation` of duty `Duty` with objective sense `obj_sense` and parent formulation
-`parent_formulation`.
+Construct a `Formulation` of duty `duty` in the environment `env`, with
+parent formulation `parent_formulation` and objective sense `obj_sense`.
 """


I would say "Create a new formulation in the Coluna's environment env with duty duty, parent formulation parent_formulation, and objective sense obj_sense.

guimarqu · 2021-03-28T17:34:24Z

src/MathProg/formulation.jl

@@ -85,6 +98,8 @@ getprimalsolcosts(form::Formulation) = form.manager.primal_sol_costs
 getdualsolmatrix(form::Formulation) = form.manager.dual_sols
 getdualsolrhss(form::Formulation) = form.manager.dual_sol_rhss

+"Generates a new `uid` to a formulation in `Env` env."
+getnewuid(env::Coluna.Env) = env.form_counter += 1


I think you can delete this method and do the operation in create_formulation.

guimarqu · 2021-03-28T17:35:24Z

src/MathProg/formulation.jl

+    nothing, NoOptimizer(), FormulationManager(),
+    MinSense, FormulationBuffer()
+)
+


This method should not exist. Do we create the original formulation before instanciating the Env ?

I think so. Isn't Env instantiated in optimize! ("optimize.jl")?

Yes, we can instanciate it when we create Optimizer, what do you think ?

Yes, that makes sense.

guimarqu · 2021-03-28T17:36:57Z

src/optimize.jl

@@ -54,7 +54,9 @@ function optimize!(prob::MathProg.Problem, annotations::Annotations, params::Par
    env = Env(params)

    # Apply decomposition
-    reformulate!(prob, annotations, env)
+    if(prob.re_formulation == nothing)


Style : no parenthesis for if statement.

You should use === to compare against nothing.

Why do you need this condition ?

Sorry, I mixed the if statement with C/C++. I need this condition because there was an error when trying to optimize twice. Maybe because a new environment is instantiated each call for optimize!, and the same doesn't happen to Problem.

haha no problem. Ok makes sense. So if we instanciate the Env and Optimizer at the same time, it will fix this issue and you will be able to remove this condition.

src/MOIwrapper.jl

update Formulation docstring

be82a5f

laradicp requested a review from guimarqu March 26, 2021 15:40

several adaptations to form_counter in Env

8c82ef6

laradicp mentioned this pull request Mar 27, 2021

Remove Counter #486

Closed

guimarqu reviewed Mar 28, 2021

View reviewed changes

laradicp and others added 2 commits March 29, 2021 12:46

fix docs and instantiate env in Optimizer

71e7097

Merge branch 'master' into formulation_docstring

3b1080a

guimarqu reviewed Mar 29, 2021

View reviewed changes

src/MOIwrapper.jl Outdated Show resolved Hide resolved

laradicp and others added 2 commits March 29, 2021 14:30

erase Formulation{Original}

f953265

Update formulation.md

55f28e2

guimarqu approved these changes Mar 29, 2021

View reviewed changes

guimarqu enabled auto-merge (squash) March 29, 2021 17:46

guimarqu modified the milestone: v0.4 Mar 29, 2021

guimarqu linked an issue Mar 29, 2021 that may be closed by this pull request

Remove Counter #486

Closed

guimarqu removed a link to an issue Mar 29, 2021

Remove Counter #486

Closed

guimarqu added the design label Mar 29, 2021

guimarqu merged commit 7a7d726 into master Mar 29, 2021

guimarqu deleted the formulation_docstring branch March 31, 2021 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Formulation docstring #484

Update Formulation docstring #484

laradicp commented Mar 26, 2021 •

edited by guimarqu

Loading

codecov bot commented Mar 26, 2021 •

edited

Loading

guimarqu commented Mar 26, 2021

laradicp commented Mar 26, 2021 •

edited

Loading

guimarqu commented Mar 26, 2021 •

edited

Loading

laradicp commented Mar 26, 2021

guimarqu commented Mar 26, 2021

guimarqu commented Mar 26, 2021 •

edited

Loading

laradicp commented Mar 27, 2021

guimarqu commented Mar 27, 2021

laradicp commented Mar 27, 2021

guimarqu commented Mar 27, 2021 via email

guimarqu left a comment

guimarqu Mar 28, 2021

guimarqu Mar 28, 2021

guimarqu Mar 28, 2021

laradicp Mar 28, 2021

guimarqu Mar 28, 2021

laradicp Mar 29, 2021

guimarqu Mar 28, 2021

laradicp Mar 28, 2021

guimarqu Mar 28, 2021

Update Formulation docstring #484

Update Formulation docstring #484

Conversation

laradicp commented Mar 26, 2021 • edited by guimarqu Loading

codecov bot commented Mar 26, 2021 • edited Loading

Codecov Report

guimarqu commented Mar 26, 2021

laradicp commented Mar 26, 2021 • edited Loading

guimarqu commented Mar 26, 2021 • edited Loading

laradicp commented Mar 26, 2021

guimarqu commented Mar 26, 2021

guimarqu commented Mar 26, 2021 • edited Loading

laradicp commented Mar 27, 2021

guimarqu commented Mar 27, 2021

laradicp commented Mar 27, 2021

guimarqu commented Mar 27, 2021 via email

guimarqu 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

laradicp commented Mar 26, 2021 •

edited by guimarqu

Loading

codecov bot commented Mar 26, 2021 •

edited

Loading

laradicp commented Mar 26, 2021 •

edited

Loading

guimarqu commented Mar 26, 2021 •

edited

Loading

guimarqu commented Mar 26, 2021 •

edited

Loading