search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 469 forks source link

Add package pyscipopt, add MIP backend #21003

Closed mkoeppe closed 1 year ago

mkoeppe commented 8 years ago

This ticket adds a package pyscipopt and adds a new MIP backend based on it.

https://github.com/scipopt/PySCIPOpt

Branch is on top of #31329.

Steps to get it to work:

Depends on #31329

CC: @mo271 @yuan-zhou @malb @dcoudert @dimpase

Component: linear programming

Keywords: days84, IMA-PolyGeom

Author: Matthias Koeppe, Moritz Firsching, Martin Rubey

Branch/Commit: 099c15b

Reviewer: Moritz Firsching, Vincent Delecroix, David Coudert, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/21003

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 82fd6d6 to 8af6e7f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

07a8254almost all doctests working, some pyscipopt patches
9206579getParam and setParam working
60b470fall methods implemented in backend; all doctests working
ccaae73one more patch
c0bc9d2pyscipopt: update to v1.4.3
04f33d1pyscipopt: update to v1.4.4
3816595scip-backend: added '# optional - pyscipopt'
563e0aaquicksum and modify doctests
59d8529remove all patches
8af6e7fchange 4 back to 9
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 8af6e7f to c82646a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

670146balmost all doctests working, some pyscipopt patches
cfbbdb1getParam and setParam working
9d66f70all methods implemented in backend; all doctests working
2b6efc9one more patch
d246de5pyscipopt: update to v1.4.3
b431129pyscipopt: update to v1.4.4
1f9050dscip-backend: added '# optional - pyscipopt'
95890afquicksum and modify doctests
6c7d9c9remove all patches
c82646achange 4 back to 9
mo271 commented 6 years ago
comment:40

now everything seems to work! I reviewed the stubs and everything written by Matthias on this ticket.

mo271 commented 6 years ago

Reviewer: Moritz Firsching

videlec commented 6 years ago
comment:41

In the spkg-install script you should not be using the make command but rather sdh_make from the "Sage-distribution helper" tools (see src/bin/sage-dist-helpers).

videlec commented 6 years ago
comment:42

Also avoid $MAKE test | tee testsuite.log (in spkg-check). It is not the role of the script to perform redirections.

mo271 commented 6 years ago
comment:43

Thanks for the comments, Vincent! I assume, they are meant for the scipoptsuite package, not the pyscipopt one, I will change that on #24662..

videlec commented 6 years ago
comment:44

Please use multiple lines for

cmake .. -DCMAKE_INSTALL_PREFIX="${SAGE_LOCAL}"/ -DCMAKE_VERBOSE_MAKEFILE=ON -DGMP_DIR="${SAGE_LOCAL}" -DZLIB_ROOT="${SAGE_LOCAL}" -DReadline_ROOT_DIR="${SAGE_LOCAL}" -DBLISS_DIR="${SAGE_LOCAL}" -DIPOPT=FALSE
videlec commented 6 years ago
comment:45

Replying to @mo271:

Thanks for the comments, Vincent! I assume, they are meant for the scipoptsuite package, not the pyscipopt one, I will change that on #24662..

You should not, this ticket is in positive review!

mo271 commented 6 years ago
comment:46

Replying to @videlec:

Replying to @mo271:

Thanks for the comments, Vincent! I assume, they are meant for the scipoptsuite package, not the pyscipopt one, I will change that on #24662..

You should not, this ticket is in positive review!

ok

videlec commented 6 years ago
comment:47

Why this change in numerical/mip.pyx

-        self._p._backend.set_variable_type(j, self._vtype)
+        #self._p._backend.set_variable_type(j, self._vtype)

If this line is not needed anymore just remove it.

videlec commented 6 years ago
comment:48

You could also simplify the SPKG.txt file:

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from c82646a to 31e0d1f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

c3bc320getParam and setParam working
c4c3680all methods implemented in backend; all doctests working
d3aa9d0one more patch
e9f531epyscipopt: update to v1.4.3
0a07d61pyscipopt: update to v1.4.4
de6b1bfscip-backend: added '# optional - pyscipopt'
fba294aquicksum and modify doctests
4aadef8remove all patches
94fac2dchange 4 back to 9
31e0d1frebased and Vincent's suggestions
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 31e0d1f to c0b432c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

c0b432ccorrect linebreaks
mo271 commented 6 years ago
comment:51

I addressed all suggestions by Vincent and tested the install of scipoptsuite as well as pyscipopt.

videlec commented 6 years ago
comment:52

merge conflicts

videlec commented 6 years ago

Changed reviewer from Moritz Firsching to Moritz Firsching, Vincent Delecroix

videlec commented 6 years ago
comment:53

And this will not work

cmake .. -DCMAKE_INSTALL_PREFIX="${SAGE_LOCAL}"/
         -DCMAKE_VERBOSE_MAKEFILE=ON
         -DGMP_DIR="${SAGE_LOCAL}"
         -DZLIB_ROOT="${SAGE_LOCAL}"
         -DReadline_ROOT_DIR="${SAGE_LOCAL}"
         -DBLISS_DIR="${SAGE_LOCAL}"
         -DIPOPT=FALSE

You need to add backslashes for line continuation

cmake .. -DCMAKE_INSTALL_PREFIX="${SAGE_LOCAL}" \
         -DCMAKE_VERBOSE_MAKEFILE=ON            \
         -DGMP_DIR="${SAGE_LOCAL}"              \
         -DZLIB_ROOT="${SAGE_LOCAL}"            \
         -DReadline_ROOT_DIR="${SAGE_LOCAL}"    \
         -DBLISS_DIR="${SAGE_LOCAL}"            \
         -DIPOPT=FALSE
mo271 commented 6 years ago
comment:54

I will get to work. I think the line break issue was fixed here: https://github.com/sagemath/sagetrac-mirror/commit/c0b432c427236c07f76f930a156a15d3f33eb1f1

But I can make the slashes align, of course.

videlec commented 6 years ago
comment:55

Alignment does not matter. But there should be backslashes.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

3c7d354all methods implemented in backend; all doctests working
e9095b7one more patch
f33a55epyscipopt: update to v1.4.3
99c5010pyscipopt: update to v1.4.4
78ef11dscip-backend: added '# optional - pyscipopt'
e59e041quicksum and modify doctests
24ca41dremove all patches
d627bcdchange 4 back to 9
10913f8rebased and Vincent's suggestions
c07eb7ecorrect linebreaks
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from c0b432c to c07eb7e

mo271 commented 6 years ago
comment:57

Replying to @videlec:

merge conflicts

I did a rebase, and it was done automatically. I ran all the tests, and (at least on my machine) everything works..

I am a bit confused, because I expected to see merge conflicts..

videlec commented 6 years ago
comment:58

Replying to @mo271:

Replying to @videlec:

merge conflicts

I did a rebase, and it was done automatically. I ran all the tests, and (at least on my machine) everything works..

I am a bit confused, because I expected to see merge conflicts..

Applying patches is not commutative!

videlec commented 6 years ago
comment:59

What is the point of using Cython here? Because you use pyscipopt everywhere there is no C at all in the file scip_backend.pyx. I would keep it simple and use pure python here.

mo271 commented 6 years ago
comment:61

Replying to @videlec:

What is the point of using Cython here? Because you use pyscipopt everywhere there is no C at all in the file scip_backend.pyx. I would keep it simple and use pure python here.

I think Matthias made the backend Cython, because the generic_backend.pyx is also written this way and all the other files are written in this way. However other backends might actually need this.

I agree that there we don't really need Cython for the pyscipopt backend. But it doesn't do any harm, does it? I am indifferent; we can change it or leave it, both is fine with me.

videlec commented 6 years ago
comment:62

Replying to @mo271:

Replying to @videlec:

What is the point of using Cython here? Because you use pyscipopt everywhere there is no C at all in the file scip_backend.pyx. I would keep it simple and use pure python here.

I think Matthias made the backend Cython, because the generic_backend.pyx is also written this way and all the other files are written in this way. However other backends might actually need this.

I agree that there we don't really need Cython for the pyscipopt backend. But it doesn't do any harm, does it? I am indifferent; we can change it or leave it, both is fine with me.

On the plus side

On the minus side

The reason why all other backends are written in Cython is because they do call C functions.

mkoeppe commented 6 years ago
comment:63

Replying to @mo271:

I think Matthias made the backend Cython, because the generic_backend.pyx is also written this way and all the other files are written in this way.

That's correct, I did this just for copypastability and no technical reason otherwise.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

0e30ccapyscipopt: update to v1.4.3
7e112c8pyscipopt: update to v1.4.4
d1519b9scip-backend: added '# optional - pyscipopt'
901f8cfquicksum and modify doctests
8fc35cfremove all patches
32884a0change 4 back to 9
0ef22c6rebased and Vincent's suggestions
d23ed3ecorrect linebreaks
1cefe04towards uncythening the scip backend
ae0bab0decythonizing finished
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from c07eb7e to ae0bab0

mkoeppe commented 6 years ago
comment:65

Another update to SCIPoptsuite 6.0 is due

videlec commented 6 years ago
comment:66

milestone update 8.3 -> 8.4

mkoeppe commented 3 years ago

Changed branch from u/moritz/pyscipopt to u/mkoeppe/pyscipopt

mkoeppe commented 3 years ago

Changed dependencies from #22557, #24662 to #31329

mkoeppe commented 3 years ago

Changed branch from u/mkoeppe/pyscipopt to u/moritz/pyscipopt

mkoeppe commented 3 years ago
comment:68

Squashed/rebased onto #31329 (Update scipoptsuite to 7.0.2)

mkoeppe commented 3 years ago

Changed branch from u/moritz/pyscipopt to u/mkoeppe/pyscipopt

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

cd0b761build/pkgs/pyscipopt: Update to 3.1.0, update metadata format
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from ae0bab0 to cd0b761

mkoeppe commented 3 years ago
comment:71
sage: from pyscipopt import Model                                                                                                                                                                                        
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-1-fcc6f2f967b1> in <module>
----> 1 from pyscipopt import Model

~/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python3.9/site-packages/pyscipopt/__init__.py in <module>
      9 # export user-relevant objects:
     10 from pyscipopt.Multidict import multidict
---> 11 from pyscipopt.scip      import Model
     12 from pyscipopt.scip      import Benders
     13 from pyscipopt.scip      import Benderscut

ImportError: dlopen(/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python3.9/site-packages/pyscipopt/scip.cpython-39-darwin.so, 2): Library not loaded: libscip.7.0.dylib
  Referenced from: /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python3.9/site-packages/pyscipopt/scip.cpython-39-darwin.so
  Reason: image not found
mkoeppe commented 3 years ago

Description changed:

--- 
+++ 
@@ -2,11 +2,11 @@

 https://github.com/SCIP-Interfaces/PySCIPOpt

-Branch is on top of #24662. 
+Branch is on top of #31329. 

 Steps to get it to work:
 - pull from this branch
-- put http://scip.zib.de/download.php?fname=scipoptsuite-5.0.1.tgz in `./upstream` (see #24662)
-- put [PySCIPOpt-1.4.4.tar.gz](https://files.pythonhosted.org/packages/50/2e/392ae55d3c13ef7d91eee69c764d6362e1e27f7ecceee00572f60637522c/PySCIPOpt-1.4.4.tar.gz)  in `./upstream`
+- put http://scip.zib.de/download.php?fname=scipoptsuite-7.0.2.tgz in `./upstream`
+- put PySCIPOpt-3.1.0.tar.gz in `./upstream`
 - `sage -f pyscipopt`
mkoeppe commented 3 years ago
comment:73

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

mkoeppe commented 3 years ago
comment:74

Setting a new milestone for this ticket based on a cursory review.

dimpase commented 1 year ago
comment:79

yes, I came here to note the license change too :-)

mkoeppe commented 1 year ago

Description changed:

--- 
+++ 
@@ -1,12 +1,10 @@
-This ticket adds a package pyscipopt and adds a new MIP backend based on it.
+This ticket adds a package `pyscipopt` and adds a new MIP backend based on it.

-https://github.com/SCIP-Interfaces/PySCIPOpt
+https://github.com/scipopt/PySCIPOpt

 Branch is on top of #31329. 

 Steps to get it to work:
 - pull from this branch
-- put http://scip.zib.de/download.php?fname=scipoptsuite-7.0.2.tgz in `./upstream`
-- put PySCIPOpt-3.1.0.tar.gz in `./upstream`
 - `sage -f pyscipopt`
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

3680001build/pkgs/scipoptsuite/SPKG.rst: Update
dadd03dbuild/pkgs/scipoptsuite: Update to 8.0.2, remove patches for now
19614bcbuild/pkgs/soplex: New
9eaece2Merge #34742
cbc591abuild/pkgs/scipoptsuite: Change to optional
0e1057dbuild/pkgs/scipoptsuite: Update dependencies, cmake options
148dac6Merge #31329
0c390feAdd SCIP backend, New pip package pyscipopt
f127fc6build/pkgs/pyscipopt: Update to 3.1.0, update metadata format
ebf2237build/pkgs/pyscipopt: Update to 4.2.0