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

Breaking changes for MOI v0.10 #147

Merged
merged 17 commits into from
Nov 8, 2021
Merged

Breaking changes for MOI v0.10 #147

merged 17 commits into from
Nov 8, 2021

Conversation

metab0t
Copy link
Contributor

@metab0t metab0t commented Sep 22, 2021

Hope CI is happy this time.

@odow
Copy link
Member

odow commented Sep 22, 2021

Yeah it should run now.

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #147 (f39c58e) into master (6a6cdbf) will decrease coverage by 0.18%.
The diff coverage is 83.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   54.65%   54.46%   -0.19%     
==========================================
  Files           9        9              
  Lines        4007     4030      +23     
==========================================
+ Hits         2190     2195       +5     
- Misses       1817     1835      +18     
Impacted Files Coverage Δ
src/MOI/MOI_wrapper.jl 86.69% <83.73%> (-0.77%) ⬇️
src/helper.jl 41.13% <0.00%> (-0.64%) ⬇️

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 6a6cdbf...f39c58e. Read the comment docs.

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Nice! Things are working at least. There are still a few tests being excluded, but it might be easiest to merge this PR first, and then tackle them individually.

src/MOI/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI/MOI_wrapper.jl Outdated Show resolved Hide resolved
test/MathOptInterface/MOI_Wrapper.jl Outdated Show resolved Hide resolved
test/MathOptInterface/MOI_Wrapper.jl Show resolved Hide resolved
test/MathOptInterface/MOI_Wrapper.jl Outdated Show resolved Hide resolved
test/MathOptInterface/MOI_Wrapper.jl Outdated Show resolved Hide resolved
test/MathOptInterface/MOI_Wrapper.jl Outdated Show resolved Hide resolved
@metab0t
Copy link
Contributor Author

metab0t commented Sep 22, 2021

The callback tests now all pass.
I use Revise.jl and forget to restart Julia yesterday :(

@metab0t
Copy link
Contributor Author

metab0t commented Sep 23, 2021

Conflict tests and farkas tests that exist in MOI.Test are removed.

@metab0t
Copy link
Contributor Author

metab0t commented Sep 23, 2021

The MOI test config is similar with Gurobi.jl. The conic tests throw error if MOI.ConstraintDual and MOI.DualObjectiveValue are not excluded.

@metab0t
Copy link
Contributor Author

metab0t commented Sep 23, 2021

I have a look at MOI.Test.test_solve_conflict_EqualTo.
The IIS obtained by Xpress says that b2 is not in conflict.

@joaquimg
Copy link
Member

I have a look at MOI.Test.test_solve_conflict_EqualTo.
The IIS obtained by Xpress says that b2 is not in conflict.

Xpress IIS does not track variable bounds

@joaquimg
Copy link
Member

@metab0t do you have plans to look into the failing tests?

@metab0t
Copy link
Contributor Author

metab0t commented Sep 30, 2021

@joaquimg Yes, I plan to look at these problems.
I will look at these failing tests today.

"test_objective_set_via_modify"
"test_model_ListOfConstraintAttributesSet"
"test_objective_get_ObjectiveFunction_ScalarAffineFunction"

@metab0t
Copy link
Contributor Author

metab0t commented Sep 30, 2021

@joaquimg
For test_objective_set_via_modify the error happens here
We need to change model.objective_type in

function MOI.modify(
    model::Optimizer,
    c::MOI.ConstraintIndex{MOI.ScalarAffineFunction{Float64}, <:Any},
    chg::MOI.ScalarCoefficientChange{Float64}
)

I cannot think of a good solution now.
Another error happens in line 514, the empty model should have zero attributes when calling MOI.get(model, MOI.ListOfModelAttributesSet()) and I have fixed it.

@metab0t
Copy link
Contributor Author

metab0t commented Sep 30, 2021

@joaquimg
For test_objective_get_ObjectiveFunction_ScalarAffineFunction, there is a corner case in Xpress.jl.
If the MOI.ObjectiveFunction is set but MOI.ObjectiveSense is not set, then model.is_feasibility=true and MOI.get(model::Optimizer, ::MOI.ObjectiveFunctionType) = nothing.
Should we add another field to indicate whether objective function is set because is_feasibility has been used to indicate whether objective sense is set?

@odow
Copy link
Member

odow commented Sep 30, 2021

Take a look at GLPK etc.
https://github.com/jump-dev/GLPK.jl/blob/d11ae27aaa3666217786850c564aeffa138a6dac/src/MOI_wrapper/MOI_wrapper.jl#L88-L89

I switched to having a objective_sense::Union{Nothing,MOI.OptimizationSense}.

@metab0t
Copy link
Contributor Author

metab0t commented Oct 1, 2021

Take a look at GLPK etc. https://github.com/jump-dev/GLPK.jl/blob/d11ae27aaa3666217786850c564aeffa138a6dac/src/MOI_wrapper/MOI_wrapper.jl#L88-L89

I switched to having a objective_sense::Union{Nothing,MOI.OptimizationSense}.

I have made similar changes in latest commit.

joaquimg and others added 5 commits October 2, 2021 04:15
@metab0t
Copy link
Contributor Author

metab0t commented Oct 11, 2021

@joaquimg Any plans to merge this PR and prepare a version bump?

@joaquimg
Copy link
Member

While this was opened there were a few fixes being applied in other PRs. I was waiting for them to complete so that we don't need to backport.
If nothing else show up this week I will proceed with the merge.
Thanks!

@joaquimg
Copy link
Member

joaquimg commented Nov 7, 2021

Still requires:
jump-dev/MathOptInterface.jl#1648

@joaquimg
Copy link
Member

joaquimg commented Nov 7, 2021

Now waiting for:
jump-dev/MathOptInterface.jl#1650

@joaquimg joaquimg merged commit a9bc4e0 into master Nov 8, 2021
@odow odow deleted the moi_v0.10 branch April 5, 2023 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants