add support general nonlinear cons #134

fserra · 2018-03-02T11:20:24Z

closes #41

inspired in SCIP.jl, pyomo and biogeme's expressions

rschwarz

Nice tests!

rschwarz · 2018-03-02T11:30:49Z

src/pyscipopt/expr.pxi

+# We have different types of general expressions that in addition
+# to the operation and list of children stores
+# SumExpr: coefficients and constant
+# ProdExpr: constant


Nice documentation!

But what is the meaning of the constant for ProdExpr? Is it contant * child1 * child2 * ... ?

rschwarz · 2018-03-02T11:32:04Z

src/pyscipopt/expr.pxi

@@ -11,21 +55,21 @@ def _is_number(e):

 def _expr_richcmp(self, other, op):
    if op == 1: # <=
-        if isinstance(other, Expr):
+        if isinstance(other, Expr) or isinstance(other, GenExpr):


If Expr and GenExpr are mostly handled the same, then maybe it would be easier to use a common base class for both of them?

yes, maybe, but I felt uncertain. The idea was to try to keep Expr as untouched as possible, given that it has been much more tested.

rschwarz · 2018-03-02T11:34:09Z

src/pyscipopt/expr.pxi

+# nor we simplify common terms or do any other type of simplification.
+# The `GenExpr` is pass as is to SCIP and SCIP will do what it see fits during presolving.
+#
+# TODO: All this is very complicated, so we might wanna unify Expr and GenExpr.


It seems to me like GenExpr is strictly more powerful than Expr, so Expr could be removed completely?
I don't know whether there is a difference in performance between these implementations...

In any case, this step could also be done in the future.

Well, this is not true, you can't ask information about GenExpr as you can with Expr, but if we do not care about that, then yes, it is more powerful.

I guess there are performance difference, I didn't really test this, but I would definitely do this in the future

rschwarz · 2018-03-02T11:36:44Z

src/pyscipopt/expr.pxi

+            # is no longer in place, might affect performance?
+            # can't do `self = buildGenExprObj(self) + other` since I get
+            # TypeError: Cannot convert pyscipopt.scip.SumExpr to pyscipopt.scip.Expr
+            return buildGenExprObj(self) + other


what if we do this instead:

return other + buildGenExprObj(self)

Would this be inplace (but for other's place)?

I am guessing that inplace means self's place. But yeah, this inplace stuff is a bit complicated and would also postpone it.

rschwarz · 2018-03-02T11:37:35Z

src/pyscipopt/expr.pxi

+        divisor = buildGenExprObj(other)
+        # we can't divide by 0
+        if divisor.getOp() == Operator.const:
+            assert divisor.number != 0.0


I think a Python programmer would expect a ValueError instead of an AssertionError, but I don't mind too much.

I changed this and added a test for this. thanks

rschwarz · 2018-03-02T11:40:21Z

src/pyscipopt/expr.pxi

+        '''turn it into a constraint'''
+        return _expr_richcmp(self, other, op)
+
+    def degree(self):


why even add this method?

just to keep it compatible with the implementation of addCons in scip.pxd or whathever the termination is

rschwarz · 2018-03-02T11:42:36Z

src/pyscipopt/expr.pxi

+    ans.op = Operator.exp
+    ans.operatorIndex = Operator.operatorIndexDic[ans.op]
+    ans.children.append(buildGenExprObj(expr))
+    return ans


why do set all these attributes after creating an "empty" object, instead of using the UnaryExpr.__init__ method to initialize them?

what you suggest is way more elegant, will implement after lunch ;)

Good, so I made one constructive suggestions ;-)

rschwarz · 2018-03-02T11:45:19Z

src/pyscipopt/scip.pyx

@@ -891,6 +891,8 @@ cdef class Model:
            return self._addLinCons(cons, **kwargs)
        elif deg <= 2:
            return self._addQuadCons(cons, **kwargs)
+        elif deg == float('inf'): # general nonlinear
+            return self._addGenNonlinearCons(cons, **kwargs)
        else:
            return self._addNonlinearCons(cons, **kwargs)


so this is for polynomials only?

_addNonlinearCons is for polynomials only, yes

I would rather rename it to _addPolyCons() or something similar. It's quite unintuitive right now and these are only internal methods.

rschwarz · 2018-03-02T11:47:05Z

src/pyscipopt/scip.pyx

@@ -1008,6 +1010,100 @@ cdef class Model:
        free(varexprs)
        return PyCons

+    def _addGenNonlinearCons(self, ExprCons cons, **kwargs):


I'll have to believe you on this function...

haha, this is basically a copy-paste of the one in CSIP

codecov · 2018-03-02T13:49:10Z

Codecov Report

Merging #134 into master will increase coverage by 1.54%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #134      +/-   ##
=========================================
+ Coverage   87.15%   88.7%   +1.54%     
=========================================
  Files          18      19       +1     
  Lines        1059    1204     +145     
=========================================
+ Hits          923    1068     +145     
  Misses        136     136

Impacted Files	Coverage Δ
tests/test_linexpr.py	`100% <ø> (ø)`	⬆️
src/pyscipopt/__init__.py	`100% <100%> (ø)`	⬆️
tests/test_expr.py	`100% <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 49a830d...e6227a4. Read the comment docs.

mattmilten · 2018-03-02T13:51:07Z

Shall we increase the version number to 1.4 or 2.0 after merging?

fserra · 2018-03-02T13:55:15Z

I have no idea how the versioning works

rschwarz · 2018-03-02T14:00:45Z

I have no idea how the versioning works

Apparently, the version is defined in __init__.py

mattmilten · 2018-03-02T14:14:41Z

Apparently, the version is defined in init.py

Haha, I guess, @fserra was referring to what the meaning of the version digits was - not how we can change the version. So, the question boils down to whether we want to consider this a major or a minor version jump. I say we call it 1.4 since we don't break the interface.

add support general nonlinear cons

c999721

inspired in SCIP.jl, pyomo and biogeme's expressions

fserra requested a review from rschwarz March 2, 2018 11:20

fserra assigned mattmilten Mar 2, 2018

rschwarz reviewed Mar 2, 2018

View reviewed changes

make it python3 compatible; remove old test

4a8c024

rschwarz approved these changes Mar 2, 2018

View reviewed changes

make python3 work second try

e6227a4

mattmilten merged commit acff07c into master Mar 2, 2018

mattmilten deleted the fs/general_nonlinear branch July 13, 2018 07:42

add support general nonlinear cons #134

add support general nonlinear cons #134

Uh oh!

Conversation

fserra commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschwarz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattmilten commented Mar 2, 2018

Uh oh!

fserra commented Mar 2, 2018

Uh oh!

rschwarz commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattmilten commented Mar 2, 2018

Uh oh!

Uh oh!

fserra commented Mar 2, 2018 •

edited

Loading

codecov bot commented Mar 2, 2018 •

edited

Loading

rschwarz commented Mar 2, 2018 •

edited

Loading