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.

Sign up for GitHub

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

Jump to bottom

Add example of SCIP with JuMP #145

Closed
wants to merge 5 commits into from
Closed

Add example of SCIP with JuMP #145

wants to merge 5 commits into from

Conversation

mrchaos
Copy link

@mrchaos mrchaos commented Dec 19, 2019 •
edited
Loading

all test passed

@codecov
Copy link

codecov bot commented Dec 19, 2019 •
edited
Loading

Codecov Report

Merging #145 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #145   +/-   ##
=======================================
  Coverage   88.37%   88.37%           
=======================================
  Files          21       21           
  Lines         800      800           
=======================================
  Hits          707      707           
  Misses         93       93
Impacted Files Coverage Δ
src/MOI_wrapper/results.jl 97.56% <100%> (ø) ⬆️

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 f15d471...3b3dcf4. Read the comment docs.

rschwarz
rschwarz requested changes Dec 19, 2019
View reviewed changes
Copy link
Collaborator

@rschwarz rschwarz left a comment

Choose a reason for hiding this comment

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

Thanks for the example.

I don't think we can upgrade CEnum this way. Or at least, we should then require the 0.2 version and regenerate the wrapper code. Did you have problems with 0.1?

Project.toml
@@ -8,7 +8,7 @@ Libdl = "8f399da3-3557-5675-b5ff-fb832c97cbdb"
MathOptInterface = "b8f27783-ece8-5eb3-8dc8-9495eed66fee"

[compat]
CEnum = "^0.1.0"
CEnum = "^0.1.0, 0.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what this means. Did you require CEnum in version 0.2?
I don't think it's compatible with our generated wrapper code.

Copy link
Author

Choose a reason for hiding this comment

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

It need version 0.2 or later to install SCIP interface version 7.4 above
Current CEnum version is 0.2
New SCIP interface version not installed without 0.2

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand. The newest version of SCIP.jl is 0.9.2 and the newest version of SCIP is 6.0.2. So what is this 7.4 refering to?

I did not try Julia 1.3.0 yet, so maybe it's related to that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just tried to add SCIP to a fresh Julia 1.3.0 installation, which worked just fine. It installed CEnum 0.1.2 as a dependency and the tests passed. Maybe you have some other package that requires CEnum 0.2?

Also, see #144 for failing tests with CEnum 0.2. Your changes probably fix those.

So, while it does make sense to upgrade CEnum at some point, I would prefer to do it "properly", and regenerate the C wrappers with a new Clang etc.

example/scip_with_jump_minlp.jl
@constraint(m, -1 + l[1] + 3l[2] + l[3] - l[4] + l[5] == 0)
@constraint(m, 4l[2] - 2l[2] - 3l[3] == 0)
for i in 1:5
@NLconstraint(m, l[i] * s[i] == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is quadratic, it actually doesn't need the @NLconstraint, and could use @constraint instead. But it should not matter much to SCIP.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just wanted to know if @NLcontraint works well with SCIP.
The example code refers to Chap8 in "Julia Programming for Operations Research 2 / e".

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are potential copyright issues with copying code verbatim from a book. Please clarify the license of this code.

Copy link
Author

Choose a reason for hiding this comment

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

OK~
I'll check!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what copyright license my codes carry, but I'm good with this.

Codes were mine, but the actual example is from:

Ex 9.1.1 from Floudas, C.A., Pardalos, P.M., Adjiman, C., Esposito, W.R., Gümüs, Z.H., Harding, S.T., Klepeis, J.L., Meyer, C.A. and Schweiger, C.A., 2013. Handbook of Test Problems in Local and Global Optimization (Vol. 33). Springer Science & Business Media, http://titan.princeton.edu/TestProblems/ 

src/MOI_wrapper/results.jl
@@ -40,7 +40,7 @@ function MOI.get(o::Optimizer, ::MOI.ResultCount)::Int
end

function MOI.get(o::Optimizer, ::MOI.RawStatusString)
return String(CEnum.enum_name(SCIPgetStatus(o)))
return String(Symbol(SCIPgetStatus(o)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably a consequence of upgrading CEnum?

Copy link
Author

Choose a reason for hiding this comment

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

The error occurs because CEnum does not have an enum_name field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, it has in version 0.1.0: CEnum.jl

src/wrapper/commons.jl
@@ -700,7 +700,7 @@ const SCIP_PRIMAL = SCIP_Primal
# Skipping MacroDefinition: SCIP_DECL_PROBEXITSOL ( x ) SCIP_RETCODE x ( SCIP * scip , SCIP_PROBDATA * probdata , SCIP_Bool restart )
# Skipping MacroDefinition: SCIP_DECL_PROBCOPY ( x ) SCIP_RETCODE x ( SCIP * scip , SCIP * sourcescip , SCIP_PROBDATA * sourcedata , SCIP_HASHMAP * varmap , SCIP_HASHMAP * consmap , SCIP_PROBDATA * * targetdata , SCIP_Bool global , SCIP_RESULT * result )

@cenum(SCIP_Objsense{Int32},
@cenum(SCIP_Objsense,
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of this code is auto-generated and should not be edited.

Copy link
Author

Choose a reason for hiding this comment

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

SCIP_Objsense{Int32} type is "Expr"

CEnum.jl
77 elseif !isa(T, Symbol) ==> ERROR
78 throw(ArgumentError("invalid type expression for Cenum $T"))
79 end

@rschwarz rschwarz changed the title SCIP with Julia v1.3.0, JuMP v0.20.1, SCIPOptSuite-6.0.2 Add example of SCIP with JuMP Jul 17, 2020
@matbesancon
Copy link
Member

there should probably be a separate PR for CEnum to keep things simple

@rschwarz
Copy link
Collaborator

rschwarz commented Aug 3, 2020

there should probably be a separate PR for CEnum to keep things simple

This was already done and is merged now: #175.

So, I guess this PR could be rebased on master and simplified.
We could then also consider adding another CI test stage for testing examples using JuMP.

@rschwarz rschwarz closed this Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rschwarz rschwarz rschwarz requested changes

@mlubin mlubin mlubin left review comments

@chkwon chkwon chkwon left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mrchaos @matbesancon @rschwarz @mlubin @chkwon