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

Optimize twice #462

Merged
merged 12 commits into from
Mar 9, 2021
Merged

Optimize twice #462

merged 12 commits into from
Mar 9, 2021

Conversation

laradicp
Copy link
Contributor

@laradicp laradicp commented Mar 5, 2021

No description provided.

@laradicp laradicp linked an issue Mar 5, 2021 that may be closed by this pull request
4 tasks
@guimarqu
Copy link
Contributor

guimarqu commented Mar 6, 2021

Let's focus on reformulation with direct_model = true, it looks like there is a bug because we must reinitialize parts of the Annotations data structure. This structure is used to store to which subproblem the constraints and variables belong. It is initialized when we call Coluna.Optimizer. Then, it is populated when BlockDecomposition assigns variables and constraints to subproblems, for instance :

Coluna.jl/src/MOIwrapper.jl

Lines 378 to 385 in 4d32761

function MOI.set(
model::Coluna.Optimizer, ::BD.VariableDecomposition, varid::MOI.VariableIndex,
annotation::BD.Annotation
)
store!(model.annotations, annotation, model.vars[varid])
return
end

and it is populated a last time when Coluna does the reformulation (calls to store! in buildformulation).

To fix the bug, you must keep what is stored in MOI wrapper (because the model passes through MOI only once when direct_model = true) and erase what is stored in reformulate!. I think only ann_per_form needs to be reinitialized.

@guimarqu
Copy link
Contributor

guimarqu commented Mar 6, 2021

I think for the first (with no reformulation and direct_model=true), when you write the model, the original formulation is stored in the formulation and the buffer of the subsolver. When you call optimize!, the content of the buffer is pushed in the subsolver and then erased. When you call optimize! a second time, the buffer is empty and thus the subsolver solves an empty formulation.

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #462 (4de9806) into master (8d13bfd) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
- Coverage   80.90%   80.84%   -0.06%     
==========================================
  Files          51       51              
  Lines        4351     4354       +3     
==========================================
  Hits         3520     3520              
- Misses        831      834       +3     
Impacted Files Coverage Δ
src/decomposition.jl 98.50% <100.00%> (ø)
src/optimize.jl 93.84% <100.00%> (+0.40%) ⬆️
src/Algorithm/branching/branchinggroup.jl 86.51% <0.00%> (-1.13%) ⬇️
src/Algorithm/branching/branchingalgo.jl 90.75% <0.00%> (-0.85%) ⬇️
src/Algorithm/storage.jl 73.77% <0.00%> (-0.82%) ⬇️
src/MOIcallbacks.jl 94.23% <0.00%> (-0.11%) ⬇️

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 8d13bfd...4de9806. Read the comment docs.

@laradicp laradicp marked this pull request as ready for review March 8, 2021 15:38
Copy link
Contributor

@guimarqu guimarqu left a comment

Choose a reason for hiding this comment

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

Since optimize!(model) returns nothing, you don't test if it's really working. You should retrieve the objective value & the solution of the two runs and compare them.

For the reformulation, you can use the generalized assignment (using the call to ColunaDemos such as in full instance tests).

@guimarqu guimarqu enabled auto-merge (squash) March 9, 2021 09:04
@guimarqu guimarqu merged commit 599e279 into master Mar 9, 2021
@guimarqu guimarqu deleted the optimize_twice branch March 19, 2021 18:19
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.

Cannot optimize a model twice
2 participants