Closed dimpase closed 11 years ago
vbraun
commented
12 years ago The PPL Cython wrapper should be fully functional for LP solving, its just providing a different interface than the MixedIntegerLinearProgram. I always wanted to have this more integrated but I don't have to solve plain LP's often (at least not in that language).
ptrrsn
commented
12 years ago The file trac_12533_arbitrary_precision_LP_solver_backend.patch is a PPLbackend for MixedIntegerLinearProgram. It works for all MixedIntegerLinearProgramfunctions related to general LP (excluding integer LP and binary LP) listed in http://www.sagemath.org/doc/reference/sage/numerical/mip.html, except for set_binary, set_integer, set_real, write_lp, write_mps.
A new MixedIntegerLinearProgramwith PPLas solver can be gotten by setting the solverparameter in MixedIntegerLinearProgramming's constructor to "PPL", e.g. MixedIntegerLinearProgram(solver = "PPL")
The additional feature of this backend is, it supports arbitrary precision calculations. We can get the exact optimal value by setting the exactparameter in solvemethod to True, i.e. p.solve(exact = True). Similarly, we can get the exact optimal solution by setting the exactparameter in get_valuesto True, i.e. p.get_values(exact = True, ...), which by default will return a vectorover QQ. However, we can also opt for the usual format of the optimal solution, i.e. as a dictionaryinstead of a vectorover QQ, by setting the as_vector parameter to False, e.g. p.get_values(exact = True, as_vector = False).
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
12 years ago Wow !!! Amazing patch !! O_o
Should this ticket be in "needs_review" state now ?
I have not looked deeply into your patch (well, your comment was posted 10 minutes ago :-P) and besides my 'Wow-Impressive Work" comment I had a question to ask : is PPL an optional package ? Some parts of your code behave as if it were
865 elif solver == "PPL":
866 try:
867 from sage.numerical.backends.ppl_backend import PPLBackend
868 default_solver = solver
869 except ImportError:
870 raise ValueError("PPL is not available. Please refer to the documentation to install it.") But the line you added in module_list.py makes me think it is standard. Also, that line :
for s in ["CPLEX", "GUROBI", "Coin", "GLPK", "PPL"]: has no meaning : Sage tries to find the first solver available on the machine, and GLPK is always available. Many of your methods need to be doctested and/or documented. The comments # optional - Nonexistent_LP_solver should also be replaced either by #optional - PPL if PPL is optional, and plainly removed otherwise.
Wow. Impressive work :-)
I hope I will be able to expose the GLPK function too at some point so that we would have some way to compare the results... But Wow... :-)
Nathann
dimpase
commented
12 years ago Replying to @nathanncohen:
865 elif solver == "PPL":
indeed, PPL is a standard package now (I guess this check for its availability stems from some early code, when it wasn't standard). So this check should be removed.
Then, it needs an example where one can see the benefit of the arbitrary precision (already an example with 2 variables should do). The example I got from the author (Hi, Risan!). I modified the last line, as it was following an old design:
p = MixedIntegerLinearProgram(solver = "PPL")
x = p.new_variable()
n = 100000000000000000000000000000000000000000000000000000000000000000000000000000000000000
p.set_objective(10 * n * x[1] + 50 * n * x[2])
p.add_constraint(10 * n * x[1] + 2 * n * x[2], max = 40 * n)
p.add_constraint(15 * n * x[1] + 30 * n * x[2], max = 40 * n)
print p.solve()
print p.solve(exact=True)is probably good enough.
Lastly, the parameter as_vector should be set to False by default, not to True, to be consistent with the
other LP interfaces.
ptrrsn
commented
12 years ago Hi Mr. Nathann and Prof. Dima,
Thank you!
I have finished the documentation. In addition, I have removed the redundant codes and setting the as_vector to False by default (and also fixing some bugs). In the attachment is the latest version of it.
Now, the status was changed to Need Review.
Thank you
Risan
dcoudert
commented
12 years ago I like this patch !!!
I add a brief look at the code, and I found some functions always returning True or always False in ppl_backend.pyx. I suppose it is because current version is still in devel phase (indeed it's a lot of work).
cpdef bint is_variable_binary(self, int index):
750 """
751 Test whether the given variable is of binary type.
...
768 """
769 return False
770
771 cpdef bint is_variable_integer(self, int index):
772 """
773 Test whether the given variable is of integer type.
...
789 """
790 return False
791
792 cpdef bint is_variable_continuous(self, int index):
793 """
794 Test whether the given variable is of continuous/real type.
...
811 """
812 return True
813Also, you could mention somewhere that PPL does not provide access to dual variables (my understanding from the documentation).
Nice work.
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
12 years ago David, it actually is because this solver does not support integer variables. It solves exactly, but only continuous problems. Hence these methods are actually no problem. I thought that p.new_variable(binary = True) should raise an exception, however. What this patch actually needs is a big and deep review, because it really is a very nice thing :-)
11d1fc49-71a1-44e1-869f-76be013245a0
commented
12 years ago A couple of doctests fail on the current beta (see patchbot)
dimpase
commented
12 years ago Replying to @loefflerd:
A couple of doctests fail on the current beta (see patchbot)
I have no clue where to look. Could you please provide a link?
kini
commented
12 years ago Dima - it's the big circular gray icon at the top of this page - i.e. http://patchbot.sagemath.org/ticket/12533/
I can fix this once I get to Portland if you want, also fix up some formatting issues.
dimpase
commented
12 years ago Replying to @kini:
Dima - it's the big circular gray icon at the top of this page - i.e. http://patchbot.sagemath.org/ticket/12533/
I can fix this once I get to Portland if you want, also fix up some formatting issues.
yeah, OK, indeed, I forgot...
By the way, IMHO the following doctest (in ppl.pyx) won't fly:
535 sage: MIP_Problem(0)
536 <sage.libs.ppl.MIP_Problem object at 0x7fae10fab5b8> looks very dodgy.
kini
commented
12 years ago Yeah, the memory address should be elided, of course.
ptrrsn
commented
12 years ago Thank you! I have fixed the doctesting errors.
dcoudert
commented
12 years ago Reviewer: David Coudert
dcoudert
commented
12 years ago Hello,
I can install the patch on sage-5.0.beta8 and all long tests are OK (all involved files). The documentation is build correctly and displayed properly.
However, I did some test based on the example of the optimizing_point function and got a segfault...
sage: from sage.libs.ppl import Variable, Constraint_System, MIP_Problem
sage: x = Variable(0)
sage: y = Variable(1)
sage: m = MIP_Problem()
sage: m.add_space_dimensions_and_embed(2)
sage: m.add_constraint(x >= 0)
sage: m.add_constraint(y >= 0)
sage: m.add_constraint(3 * x + 5 * y <= 10)
sage: m.set_objective_function(x + y)
sage: m.optimizing_point()
[10/3, 0]
sage: z = Variable(2)
sage: m.add_constraint(z >= -3)
terminate called after throwing an instance of 'std::invalid_argument'
what(): PPL::MIP_Problem::add_constraint(c):
c.space_dimension() == 3 exceeds this->space_dimension == 2.
...
/path-to-sage/sage-5.0.beta8/spkg/bin/sage: line 308: 29045 Aborted (core dumped) sage-ipython "$@" -iSome tests should be added somewhere.
This will be a great patch!
ptrrsn
commented
12 years ago Replying to @dcoudert:
Hello,
I can install the patch on sage-5.0.beta8 and all long tests are OK (all involved files). The documentation is build correctly and displayed properly.
However, I did some test based on the example of the
optimizing_pointfunction and got a segfault...sage: from sage.libs.ppl import Variable, Constraint_System, MIP_Problem sage: x = Variable(0) sage: y = Variable(1) sage: m = MIP_Problem() sage: m.add_space_dimensions_and_embed(2) sage: m.add_constraint(x >= 0) sage: m.add_constraint(y >= 0) sage: m.add_constraint(3 * x + 5 * y <= 10) sage: m.set_objective_function(x + y) sage: m.optimizing_point() [10/3, 0] sage: z = Variable(2) sage: m.add_constraint(z >= -3) terminate called after throwing an instance of 'std::invalid_argument' what(): PPL::MIP_Problem::add_constraint(c): c.space_dimension() == 3 exceeds this->space_dimension == 2. ... /path-to-sage/sage-5.0.beta8/spkg/bin/sage: line 308: 29045 Aborted (core dumped) sage-ipython "$@" -iSome tests should be added somewhere.
Hi, thanks for your feedback!
It is actually not a bug. The statement "m.add_space_dimensions_and_embed(2)" increases the space dimension of the MIP_Problem from 0 (default) to 2. However, z is a variable in the third dimension (the number 2 in "z = Variable(2)" means the dimension of this variable is 2, which means it is in the third dimension since the numbering starts from zero). Since the dimension of the variable exceeds the space dimension of the MIP_Problem, it gives error.
An extra line should be added to make it works: sage: z = Variable(2) sage: m.add_space_dimensions_and_embed(1) sage: m.add_constraint(z >= -3)
This will be a great patch!
Thank you!
dcoudert
commented
12 years ago It's not only giving an error, it crashed my sage session.
Wouldn't it be possible to add some test to prevent such behavior? I think it would be much smarter than a crash.
ptrrsn
commented
12 years ago Replying to @dcoudert:
It's not only giving an error, it crashed my sage session. Wouldn't it be possible to add some test to prevent such behavior? I think it would be much smarter than a crash.
I see. Yes, it is indeed troublesome. Now I realize that I also have not given enough exception handling in many other parts. I will fix them.
Thank you.
vbraun
commented
12 years ago Its a bit tricky because the PPL_Generator C++ class has a private ctor and declaring Cython exceptions while returning a const reference has a bug. I'm pretty sure we need to copy the returned PPL_Generator in optimize_point and other functions.
Also, can we make optimize_point return a PPL point and not a list of gmp rationals? We needlessly deviate from the PPL api here.
vbraun
commented
12 years ago Initial patch
vbraun
commented
12 years ago Attachment: trac_12533_catch_ppl_exception.patch.gz
I've added a patch that catches the C++ exceptions and shows how to work around Cython/C++ issues in optimizing_point().
Things that need to be done:
MIP_Problem should derive from _mutable_or_immutable just like the polyhedron class. All mutating methods need to check that the MIP_Problem is mutable.ptrrsn
commented
12 years ago Replying to @vbraun:
I've added a patch that catches the C++ exceptions and shows how to work around Cython/C++ issues in
optimizing_point().
Wow, that is a great help. Thank you!
jdemeyer
commented
12 years ago 6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
12 years ago Helloooooooo Risan !!!
I just went over your patch, and some comments follow. Thank you very much for the work you put into that ! :-)
ppl.pyx
sage/libs/ppl_ files
:-/mip.pyx
O_oppl_backend.pyx
print_data. Actually sage -coverage <filename> should never have to complain :-)Others
Thank you again :-)
Nathann
kini
commented
12 years ago apply to $SAGE_ROOT/devel/sage
kini
commented
12 years ago Attachment: trac_12533-rebased.patch.gz
I rebased Risan's patch to 5.0.rc1. I also removed trailing whitespace from the patch.
dimpase
commented
12 years ago Replying to @nathanncohen:
Hell([o]10) Nathann [!]5
ppl.pyx
- MIPProblem : PPL can only solve continuous Linear Programs, can't it ? In this case why "MIP*", unless it means something different from "Mixed Integer Program" ?
We only have MixedIntegerLinearProgram, but no LinearProgram, right?
While the latter might be a meaningful addition, it is certainly beyond the scope of this ticket, I think...
[...]
- The patch does not apply on 5.0-rc1, so I can not even check whether tests pass !
now it does, thanks to Keshav's rebasing.
Dima
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
12 years ago We only have
MixedIntegerLinearProgram, but noLinearProgram, right?
Indeed, but the file ppl.pyx (both code and documentation) contains many occurrences of "MIP" that are just misleading.
Nathann
ptrrsn
commented
12 years ago Replying to @nathanncohen:
Helloooooooo Risan !!!
Hi Mr. Nathann
I just went over your patch, and some comments follow. Thank you very much for the work you put into that !
:-)
Thank you for your review.
ppl.pyx
- MIPProblem : PPL can only solve continuous Linear Programs, can't it ? In this case why "MIP*", unless it means something different from "Mixed Integer Program" ?
It was named MIP_Problem simply because it is derived from MIP_Problem class in PPL. Since this functionality belongs to PPL, I left the name unchanged.
- MIP_Problem.optimization_mode() prints something but does not return anything
Fixed. Now it returns a string.
- MIP_Problem returns dictionaries : in MixedIntegerLinearProgram we raise an exception for unbounded/infeasible problems, and return the objective's value otherwise. Just mentionning it in case you may like it, of course that's totally up to you ! I still think it would be easier to return, for instance, a string (unbounded/infeasible) when the problem has no solution, and the result otherwise.
Fixed. Now it raises an error (ValueError) when the problem has no solution. I think it is more consistent to the PPL MIP_Problem now.
- optimal_value does not only return the objective value, it also solves the problem ! I think the docstring should be updated --> if the users calls this function several times, are the computations made over and over ?
Fixed. It does not solve the problem now.
- optimization_mode and set_optimization_mode : we often do both in a unique function, i.e. a method with optional arguments such that optimization_mode() returns the mode and optimization_mode(-1) defines it as a minimization problem. I just noticed that it was not the case at all for MixedIntegerLinearProgram methods, but I think we ought to change that :-)
This is made to be consistent with the implementation in PPL. In PPL, they used optimization_mode() and set_optimization_mode() instead of one function only :)
- is_satisfiable : does it also solve the problem ? I mean, if users type "if p.is_satisfiable(): a = p.optimal_value()" does it solve it twice ? The docstrings should say something about that.
The is_satisfiable in my wrapper wraps exactly the is_satisfiable of PPL. After looking at the PPL implementation of this function, I guess it does not solve the problem.
- what is the difference between optimal_value and evaluate_objective_function ? In particular the code seems to be pretty similar, so one function should probably call the other if two are necessary
Evaluate objective function return the evaluation of objective function on a given point (the parameter of the function). The optimal_value return the evaluation of objective function on the optimal solution.
- OK : by looking at the docstring I have no idea what it does or what the invariants are, but of course it may also be that I am not part of the intended audience for this class
I think it was used for debugging (I'm not really sure too).
sage/libs/ppl_ files
- Are these files part of the ppl spkg ? If so they should not be modified by a patch, and the spkg itself should be updated.. That's really a lot of work for nothing, but that's how things are done here until we change it
:-/
The files pp_shim.cc and ppl_shim.hh are created by vbraun (cmiiw), I actually don't have any idea about them.
mip.pyx
- Why would you have to add anything to get_values and solve ? The flags you add would create problems instantly if the backend solver is not PPL !! I think nothing needs to be changed in that file --> if the backend is ppl then the values returned will be exact, otherwise they will just be the usual ones. But I do not see why these functions should be changed
O_o
Thanks, I have changed this part. No additional flags needed anymore.
ppl_backend.pyx
- All functions should be documented/tested, even 'cutsom ones' like print_data . Actually sage -coverage
should never have to complain :-)
oops, print_data is debugging function. I forgot to delete it. Now I have deleted it.
- same thing with init_mip
It is documented now.
- add_variable[s] say that the variable are positive by default. Are they indeed ?
Yes. They are. The lower_bound is set to 0.0 by default.
- "set_variable_type" -- I think the best is for this method's code to be something like "raise Exception('This backend does not handle integer variables ! Read the doc !')"
Fixed.
- why do you keep "exact" in the names of your get_values functions ? It would be easier to name then get_values, as anyway all the values given by the ppl interface are exact ?!
Fixed.
Others
- Is ppl.pyx part of the documentation ? At least ppl_backend is not, so you should add it to doc/en/reference/numerical.rst
- Some "advertisement" should be added to the doc. In many places the solvers are listed (ok, perhaps we should only list them at one place), so that users can find out that exact answer are returned when the solver is ppl
ok. I'm still working on them and I will update it later.
- Variables : are they automatically strictly positive ? This is the default in the MIP class (because it is the default of the API we interface), and if PPL acts any differently this would mean that results would be different according to whether PPL or GLPK is used as a backend !
This is from GenericBackend (and PPL Backend inherits from it) class: "cpdef int add_variable(self, lower_bound=0.0, ..." I think the variables are non-negative by default.
- The patch does not apply on 5.0-rc1, so I can not even check whether tests pass ! Thank you again
:-)Nathann
Thanks a lot again, Mr. Nathann :)
Risan
kini
commented
12 years ago I'll rebase this later
kini
commented
12 years ago Work Issues: rebase on 5.1.beta0
kini
commented
12 years ago apply to $SAGE_ROOT/devel/sage
kini
commented
12 years ago 6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
12 years ago (Does it fail to compile on beta1 or is it just me ? It applied fine though !)
dimpase
commented
12 years ago Replying to @nathanncohen: indeed, I also get
...
Error compiling Cython file:
------------------------------------------------------------
...
if coeff is not None:
self.objective_function[variable] = coeff;
else:
return self.objective_function[variable]
cpdef set_objective(self, list coeff):
^
------------------------------------------------------------
sage/numerical/backends/ppl_backend.pyx:273:11: Signature not compatible with previous declaration
Error compiling Cython file:
------------------------------------------------------------
...
cpdef int add_variable(self, lower_bound=*, upper_bound=*, binary=*, continuous=*, integer=*, obj=*, name=*) except -1
cpdef int add_variables(self, int, lower_bound=*, upper_bound=*, binary=*, continuous=*, integer=*, obj=*, names=*) except -1
cpdef set_variable_type(self, int variable, int vtype)
cpdef set_sense(self, int sense)
cpdef objective_coefficient(self, int variable, coeff=*)
cpdef set_objective(self, list coeff, double d=*)
^
------------------------------------------------------------
sage/numerical/backends/generic_backend.pxd:14:24: Previous declaration is here
Error running command, failed with status 256.
Error installing modified sage library code.Hi,
I have fixed the compilation error and some other incompatibilities with sage 5.1. If you are using sage 5.1, please use "trac_12533_arbitrary_precision_LP_solver_backend_for_5.1.patch". If you are using sage 5.0, please use "trac_12533_arbitrary_precision_LP_solver_backend.patch".
Thank you!
ptrrsn
commented
12 years ago dimpase
commented
12 years ago Replying to @ptrrsn:
Hi,
I have fixed the compilation error and some other incompatibilities with sage 5.1. If you are using sage 5.1, please use "trac_12533_arbitrary_precision_LP_solver_backend_for_5.1.patch". If you are using sage 5.0, please use "trac_12533_arbitrary_precision_LP_solver_backend.patch".
Thanks, it works now with the latest Sage beta (on OSX 10.6.8).
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
12 years ago Hi Mr. Nathann
Hello Mister Risan !!
- MIPProblem : PPL can only solve continuous Linear Programs, can't it ? In this case why "MIP*", unless it means something different from "Mixed Integer Program" ?
It was named MIP_Problem simply because it is derived from MIP_Problem class in PPL. Since this functionality belongs to PPL, I left the name unchanged.
Oh. Then do you mean that PPL can solve integer programs too ? O_o
If its name is MIP_problem, I guess it can... Well, why don't we expose it then ???
- is_satisfiable : does it also solve the problem ? I mean, if users type "if p.is_satisfiable(): a = p.optimal_value()" does it solve it twice ? The docstrings should say something about that.
The is_satisfiable in my wrapper wraps exactly the is_satisfiable of PPL. After looking at the PPL implementation of this function, I guess it does not solve the problem.
Hmmmm.. I would be surprised if it were easier to check that the problem is satisfiable than to actually solve it. O_o.
But then again, I am often surprised.
Evaluate objective function return the evaluation of objective function on a given point (the parameter of the function). The optimal_value return the evaluation of objective function on the optimal solution.
Right. I do not know what I could do with evaluate_objective_function, but if you think that it is useful...
- OK : by looking at the docstring I have no idea what it does or what the invariants are, but of course it may also be that I am not part of the intended audience for this class
I think it was used for debugging (I'm not really sure too).
Well, then... What about removing it from the patch ?...
sage/libs/ppl_ files
The files pp_shim.cc and ppl_shim.hh are created by vbraun (cmiiw), I actually don't have any idea about them.
Ok, they seem to be included in Sage's source code, so it is actually ok to modify them
mip.pyx
- Why would you have to add anything to get_values and solve ? The flags you add would create problems instantly if the backend solver is not PPL !! I think nothing needs to be changed in that file --> if the backend is ppl then the values returned will be exact, otherwise they will just be the usual ones. But I do not see why these functions should be changed
O_oThanks, I have changed this part. No additional flags needed anymore.
Well. I saw that you now have two functions doing exactly the same thing following each other in the file : get_variable_value and get_exact_variable_value. As they seem to do the same thing (tell me if I am wrong) it should only appear once. If you want to have both available, one can be made to be a shortcut toward the other by typing in the code
get_exact_variable_value = get_variable_value
But why would you want to have both available ?
In particular, why do you still modify the file mip.pyx ? As the function get_variable_value exists, there is no need to test whether the ppl solver is being used, or to have an exact variable, or to add things like that to the code :
import sys
from sage.all import * ppl_backend.pyx
about documentation : I just remembered that you should also add a mention of ppl in the file doc/en/thematic_tutorials/linear_programming.rst near the other solvers.
- "set_variable_type" -- I think the best is for this method's code to be something like "raise Exception('This backend does not handle integer variables ! Read the doc !')"
Fixed.
Well... But it the solver actually supports integer variables...
Others
- Is ppl.pyx part of the documentation ? At least ppl_backend is not, so you should add it to doc/en/reference/numerical.rst
Well... This is not done in your new patch.
ok. I'm still working on them and I will update it later.
did you ?
Nathann
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
12 years ago Oh, and I forgot to add that all import from the PPL libraries should be done in a .pxd file (like for ppl_backend.pxd) instead :-)
Nathann
dimpase
commented
12 years ago Changed work issues from rebase on 5.1.beta0 to see the latest comments
ptrrsn
commented
12 years ago Attachment: trac_12533_arbitrary_precision_LP_solver_backend_for_5.1.patch.gz
for Sage 5.1.beta1
ptrrsn
commented
12 years ago Replying to @nathanncohen:
Hi Mr. Nathann
Hello Mister Risan !!
Hi
- MIPProblem : PPL can only solve continuous Linear Programs, can't it ? In this case why "MIP*", unless it means something different from "Mixed Integer Program" ?
It was named MIP_Problem simply because it is derived from MIP_Problem class in PPL. Since this functionality belongs to PPL, I left the name unchanged.
Oh. Then do you mean that PPL can solve integer programs too ?
O_oIf its name is MIP_problem, I guess it can... Well, why don't we expose it then ???
Because this patch is initially only intended to be an additional exact LP solver backend for general LP.
- is_satisfiable : does it also solve the problem ? I mean, if users type "if p.is_satisfiable(): a = p.optimal_value()" does it solve it twice ? The docstrings should say something about that.
The is_satisfiable in my wrapper wraps exactly the is_satisfiable of PPL. After looking at the PPL implementation of this function, I guess it does not solve the problem.
Hmmmm.. I would be surprised if it were easier to check that the problem is satisfiable than to actually solve it.
O_o. But then again, I am often surprised.
Hmm, I am not really sure about it either. It was not mentioned in the PPL documentation.
Evaluate objective function return the evaluation of objective function on a given point (the parameter of the function). The optimal_value return the evaluation of objective function on the optimal solution.
Right. I do not know what I could do with evaluate_objective_function, but if you think that it is useful...
- OK : by looking at the docstring I have no idea what it does or what the invariants are, but of course it may also be that I am not part of the intended audience for this class
I think it was used for debugging (I'm not really sure too).
Well, then... What about removing it from the patch ?...
Hmm, after checking this again, I think it is not just for debugging purpose. For example: The MIP_Problem constructor will accept any kind of constraints (including strict inequalities which violate the invariants). However, there is no checking for this in the constructor, that is why the OK() can be used to check this.
sage/libs/ppl_ files
The files pp_shim.cc and ppl_shim.hh are created by vbraun (cmiiw), I actually don't have any idea about them.
Ok, they seem to be included in Sage's source code, so it is actually ok to modify them
mip.pyx
- Why would you have to add anything to get_values and solve ? The flags you add would create problems instantly if the backend solver is not PPL !! I think nothing needs to be changed in that file --> if the backend is ppl then the values returned will be exact, otherwise they will just be the usual ones. But I do not see why these functions should be changed
O_oThanks, I have changed this part. No additional flags needed anymore.
Well. I saw that you now have two functions doing exactly the same thing following each other in the file : get_variable_value and get_exact_variable_value. As they seem to do the same thing (tell me if I am wrong) it should only appear once. If you want to have both available, one can be made to be a shortcut toward the other by typing in the code get_exact_variable_value = get_variable_value But why would you want to have both available ?
I have changed the get_variable_value to raise notimplementederror. The reason I made new functions (getexact*) is because in generic_backend, the return type of get_variable_value and get_objectivevalue are fixed to be double. Since PPLBackend inherits these methods from GenericBackend, the implementations of them inside PPLBackend should return doubles too... except if I modify the GenericBackend class (change cdef double get... to cdef get_...). What do you think?
In particular, why do you still modify the file mip.pyx ? As the function get_variable_value exists, there is no need to test whether the ppl solver is being used, or to have an
exactvariable, or to add things like that to the code :import sys from sage.all import *ppl_backend.pyx
about documentation : I just remembered that you should also add a mention of ppl in the file doc/en/thematic_tutorials/linear_programming.rst near the other solvers.
- "set_variable_type" -- I think the best is for this method's code to be something like "raise Exception('This backend does not handle integer variables ! Read the doc !')"
Fixed.
Well... But it the solver actually supports integer variables...
Others
- Is ppl.pyx part of the documentation ? At least ppl_backend is not, so you should add it to doc/en/reference/numerical.rst
Well... This is not done in your new patch.
ok. I'm still working on them and I will update it later.
did you ?
Ok, fixed.
Nathann
dimpase
commented
12 years ago Changed reviewer from David Coudert to David Coudert, Nathann Cohen
dimpase
commented
12 years ago Replying to @nathanncohen: the name unchanged.
Oh. Then do you mean that PPL can solve integer programs too ?
O_oIf its name is MIP_problem, I guess it can... Well, why don't we expose it then ???
yes, this would be great, but this ought to be another ticket...
vbraun
commented
12 years ago To the best of my knowledge PPL does not do integer programming. It technically works with integers (and an overall divisor) so that you can potentially use machine integers for speed. Though in Sage we only compile support for GMP/MPIR coefficients in. But you can't solve integer (or mixed integer) programs.
Also, Sage seems to have a MixedIntegerLinearProgram but no IntegerLinearProgram or just LinearProgram. Why is there no class hierarchy?
vbraun
commented
12 years ago Never mind, I just looked up the PPL docs with respect to MIP_Problem.
vbraun
commented
12 years ago Generally speaking, looks good! There are a few nitpicks:
In #12553, I modified the PPL classes to derive from SageObject. This'll make it play nicer with the rest of Sage. Correspondingly you should override _repr_, not __repr__. Also needs to dump the actual information of the MIP_problem or its useless in other doctests. Speaking of which, the method needs a doctest. Also, in English you'd say "A MIP" not "An MIP", but really the output should be more specific.
All expensive operations (that need to solve a LP, say) must be wrapped in sig_on/sig_off blocks. Be careful with exceptions thrown, use try/finally if necessary. Otherwise you'll get into expensive but untinterruptable computations...
Can we have a few less empty lines? No need for an empty line before and after the """ closing the docstring.
The INPUT: section should be formatted like
- ``number`` (integer) -- description.You often have only a single "-" between type and description.
Coverage should be 100%:
[vbraun@volker-desktop hg]$ sage -coverage sage/numerical/backends/ppl_backend.pyx
----------------------------------------------------------------------
sage/numerical/backends/ppl_backend.pyx
ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE sage/numerical/backends/ppl_backend.pyx: 72% (24 of 33)
Missing documentation:
* int add_variable(self, lower_bound=0.0, upper_bound=None, binary=False, continuous=True, integer=False, obj=0.0, name=None) except -1: """ Add a variable. This amounts to adding a new column to the matrix. By default, the variable is both positive and real. It has not been implemented for selecting the variable type yet. INPUT: - ``lower_bound`` - the lower bound of the variable (default: 0) - ``upper_bound`` - the upper bound of the variable (default: ``None``) - ``binary`` - ``True`` if the variable is binary (default: ``False``). - ``continuous`` - ``True`` if the variable is binary (default: ``True``). - ``integer`` - ``True`` if the variable is binary (default: ``False``). - ``obj`` - (optional) coefficient of this variable in the objective function (default: 0.0) - ``name`` - an optional name for the newly added variable (default: ``None``). OUTPUT: The index of the newly created variable EXAMPLE:: sage: from sage.numerical.backends.generic_backend import get_solver sage: p = get_solver(solver = "PPL") sage: p.ncols() 0 sage: p.add_variable() 0 sage: p.ncols() 1 sage: p.add_variable(lower_bound=-2.0) 1 sage: p.add_variable(name='x',obj=1.0) 2 sage: p.col_name(2) 'x' sage: p.objective_coefficient(2) 1.00000000000000 """ for i in range(len(self.Matrix)):
* int add_variables(self, int n, lower_bound=0.0, upper_bound=None, binary=False, continuous=True, integer=False, obj=0.0, names=None) except -1: """ Add ``n`` variables. This amounts to adding new columns to the matrix. By default, the variables are both positive and real. It has not been implemented for selecting the variable type yet. INPUT: - ``n`` - the number of new variables (must be > 0) - ``lower_bound`` - the lower bound of the variable (default: 0) - ``upper_bound`` - the upper bound of the variable (default: ``None``) - ``binary`` - ``True`` if the variable is binary (default: ``False``). - ``continuous`` - ``True`` if the variable is binary (default: ``True``). - ``integer`` - ``True`` if the variable is binary (default: ``False``). - ``obj`` - (optional) coefficient of all variables in the objective function (default: 0.0) - ``names`` - optional list of names (default: ``None``) OUTPUT: The index of the variable created last. EXAMPLE:: sage: from sage.numerical.backends.generic_backend import get_solver sage: p = get_solver(solver = "PPL") sage: p.ncols() 0 sage: p.add_variables(5) 4 sage: p.ncols() 5 sage: p.add_variables(2, lower_bound=-2.0, names=['a','b']) 6 """ for k in range(n):
* int solve(self) except -1: """ Solve the problem. .. NOTE:: This method raises ``MIPSolverException`` exceptions when the solution can not be computed for any reason (none exists, or the LP solver was not able to find it, etc...) EXAMPLE:: sage: from sage.numerical.backends.generic_backend import get_solver sage: p = get_solver(solver = "PPL") sage: p.add_linear_constraints(5, 0, None) sage: p.add_col(range(5), range(5)) sage: p.solve() 0 sage: p.objective_coefficient(0,1) sage: p.solve() Traceback (most recent call last):
Missing doctests:
* set_variable_type(self, int variable, int vtype):
* set_verbosity(self, int level):
* double get_objective_value(self):
* double get_variable_value(self, int variable):
* write_lp(self, char * name):
* write_mps(self, char * name, int modern):Finally, the ticket description should say which patch to apply for the sanity of the release manager.
Also, ptrrsn_1 should put his name on the trac author list (http://trac.sagemath.org) and populate the Author: field on the ticket.
vbraun
commented
12 years ago Dependencies: 12553
dimpase
commented
12 years ago Please also see a related #13142, which points out to the fact that show() would eventually need more work to be perfect for arbitrary precision.
There is currently no arbitrary precision LP solver backend available. It is sorely missed in some coding theory -related LP computations, see #12418.
One option is to hook up PPL, another might be to hook up GLPK for this purpose. Both have the corresponding functionality, it's just not exposed (?) in P(C)ython for GLPK, and not hooked up as an LP backend in case of PPL.
To try this patch, apply attachment: trac_12533_arbitrary_precision_LP_solver_backend_for_5.3_vb.patch
Depends on #13646
CC: @sagetrac-jpang @kini @wdjoyner @nathanncohen @ppurka @vbraun @ptrrsn @dcoudert
Component: linear programming
Keywords: arbitrary precision
Author: Risan
Reviewer: David Coudert, Nathann Cohen, Dmitrii Pasechnik
Merged: sage-5.5.beta2
Issue created by migration from https://trac.sagemath.org/ticket/12533