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

upgrade to MOI 10 #203

Merged
merged 16 commits into from
Sep 27, 2021
Merged

upgrade to MOI 10 #203

merged 16 commits into from
Sep 27, 2021

Conversation

matbesancon
Copy link
Member

Still some tests to fix, some probably been move around.

@matbesancon
Copy link
Member Author

Some tests are a bit weird to fix (what is the solver supposed to do when a BadModel cannot be copied? there is no docstring)
duplicate_VariableName feels like it should not be run when VariableName was excluded.

@matbesancon
Copy link
Member Author

Still some strange things with indicator constraints:

#     test_constraint_Indicator_ACTIVATE_ON_ZERO: Error During Test at /home/mbesancon/.julia/packages/MathOptInterface/Evu1m/src/Test/test_constraint.jl:868
#   Test threw exception
#   Expression: MOI.get(model, MOI.ConstraintSet(), c) == s
#   ArgumentError: Bridge of type `MathOptInterface.Bridges.Constraint.IndicatorActiveOnFalseBridge{Float64, MathOptInterface.VectorAffineFunction{Float64}, MathOptInterface.GreaterThan{Float64}}` does not support accessing the attribute `MathOptInterface.ConstraintSet()`. If you encountered this error unexpectedly, it probably means your model has been reformulated using the bridge, and you are attempting to query an attribute that we haven't implemented yet for this bridge. Please open an issue at https://github.com/jump-dev/MathOptInterface.jl/issues/new and provide a reproducible example explaining what you were trying to do.

to check later

@matbesancon
Copy link
Member Author

OK we are getting close to a mergeable state, I removed the bound tests for now

@matbesancon
Copy link
Member Author

@odow in the two commits starting with "remove bound" above, I am not sure if this was the intended behavior or not.

@matbesancon
Copy link
Member Author

I'd like to see if there are some containers we can simplify, if feels like we have a ton of dictionaries storing various constraint and variable indices, never sure which one should be checked

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
matbesancon and others added 3 commits September 26, 2021 00:04
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
append!(excluded, [
"test_linear_Interval_inactive",
"test_linear_integration",
"test_model_ordered_indices", # TODO should fix? ListOf in order of creation
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this needs to be fixed.

I usually add a comment why we're excluding tests as well. test_linear_integration is a fairly important test, so it'd be nice to know why it's skipped.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed here. I added an explanation for the integration test, deleting a variable in SCIP is only allowed in certain stages so we are fairly conservative in the wrapper

@rschwarz
Copy link
Collaborator

I'd like to see if there are some containers we can simplify, if feels like we have a ton of dictionaries storing various constraint and variable indices, never sure which one should be checked

Not sure what you mean. In SCIPData, there's only one Dict each for variables and constraints.

In the Optimizer struct, there are more dictionaries, but these store different values (e.g. bounds, starting point).

@matbesancon matbesancon merged commit 8e277d4 into master Sep 27, 2021
@matbesancon matbesancon deleted the fix-moi10 branch September 27, 2021 09:12
@matbesancon
Copy link
Member Author

yes the confusion was more in some parts on what dict is supposed to be used for checking that something exists

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