-
Notifications
You must be signed in to change notification settings - Fork 280
Speed up -SumExpr, -ProdExpr and -Constant
#1179
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
base: master
Are you sure you want to change the base?
Changes from 8 commits
28d3adc
1be74c8
e4351fa
fb9fcc8
f55c222
cb349ce
5896012
6387cfa
eac8db9
fecba06
02e32b5
bd280f6
2e97cc7
67ce45d
40945ad
ef034c4
86678e2
394c682
d348d48
b881b12
39c036c
3d2bff0
1c6598f
2b0cc32
70ef34e
05dd131
af2f83a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,9 +45,11 @@ | |
| import math | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| from pyscipopt.scip cimport Variable, Solution | ||
| from cpython.dict cimport PyDict_Next | ||
| from cpython.array cimport array, clone | ||
| from cpython.dict cimport PyDict_Next, PyDict_SetItem | ||
| from cpython.object cimport Py_TYPE | ||
| from cpython.ref cimport PyObject | ||
| from pyscipopt.scip cimport Variable, Solution | ||
|
|
||
| import numpy as np | ||
|
|
||
|
|
@@ -56,6 +58,9 @@ if TYPE_CHECKING: | |
| double = float | ||
|
|
||
|
|
||
| cdef array DOUBLE_TEMPLATE = array("d") | ||
|
|
||
|
|
||
| def _is_number(e): | ||
| try: | ||
| f = float(e) | ||
|
|
@@ -308,8 +313,15 @@ cdef class Expr: | |
| else: | ||
| raise TypeError(f"Unsupported base type {type(other)} for exponentiation.") | ||
|
|
||
| def __neg__(self): | ||
| return Expr({v:-c for v,c in self.terms.items()}) | ||
| def __neg__(self) -> Expr: | ||
| cdef dict res = {} | ||
| cdef Py_ssize_t pos = <Py_ssize_t>0 | ||
| cdef PyObject* key_ptr | ||
| cdef PyObject* val_ptr | ||
|
|
||
| while PyDict_Next(self.terms, &pos, &key_ptr, &val_ptr): | ||
| PyDict_SetItem(res, <Term>key_ptr, -<double>(<object>val_ptr)) | ||
| return Expr(res) | ||
|
|
||
| def __sub__(self, other): | ||
| return self + (-other) | ||
|
|
@@ -647,6 +659,21 @@ cdef class GenExpr: | |
| '''returns operator of GenExpr''' | ||
| return self._op | ||
|
|
||
| cdef GenExpr copy(self, bool copy = True): | ||
| cdef object cls = <type>Py_TYPE(self) | ||
| cdef GenExpr res = cls.__new__(cls) | ||
| res._op = self._op | ||
| res.children = self.children.copy() if copy else self.children | ||
| if cls is SumExpr: | ||
| self = <SumExpr>self | ||
| res = <SumExpr>res | ||
| res.constant = self.constant | ||
| res.coefs = clone(self.coefs, len(self.coefs), False) if copy else self.coefs | ||
| if cls is ProdExpr: | ||
| (<ProdExpr>res).constant = (<ProdExpr>self).constant | ||
| elif cls is PowExpr: | ||
| (<PowExpr>res).expo = (<PowExpr>self).expo | ||
| return res | ||
|
|
||
| # Sum Expressions | ||
| cdef class SumExpr(GenExpr): | ||
|
|
@@ -656,9 +683,24 @@ cdef class SumExpr(GenExpr): | |
|
|
||
| def __init__(self): | ||
| self.constant = 0.0 | ||
| self.coefs = [] | ||
| self.coefs = array("d") | ||
| self.children = [] | ||
|
Zeroto521 marked this conversation as resolved.
|
||
| self._op = Operator.add | ||
|
|
||
| def __neg__(self) -> SumExpr: | ||
| cdef int i = 0, n = len(self.coefs) | ||
| cdef array coefs = clone(DOUBLE_TEMPLATE, n, False) | ||
| cdef double[:] dest_view = coefs | ||
| cdef double[:] src_view = self.coefs | ||
|
|
||
| for i in range(n): | ||
| dest_view[i] = -src_view[i] | ||
|
|
||
|
Comment on lines
+718
to
+724
|
||
| cdef SumExpr res = <SumExpr>self.copy() | ||
|
Zeroto521 marked this conversation as resolved.
Outdated
|
||
| res.constant = -res.constant | ||
| res.coefs = coefs | ||
| return res | ||
|
|
||
| def __repr__(self): | ||
| return self._op + "(" + str(self.constant) + "," + ",".join(map(lambda child : child.__repr__(), self.children)) + ")" | ||
|
|
||
|
|
@@ -682,6 +724,11 @@ cdef class ProdExpr(GenExpr): | |
| self.children = [] | ||
| self._op = Operator.prod | ||
|
|
||
| def __neg__(self) -> ProdExpr: | ||
| cdef ProdExpr res = <ProdExpr>self.copy() | ||
| res.constant = -res.constant | ||
|
Zeroto521 marked this conversation as resolved.
Outdated
|
||
| return res | ||
|
|
||
| def __repr__(self): | ||
| return self._op + "(" + str(self.constant) + "," + ",".join(map(lambda child : child.__repr__(), self.children)) + ")" | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Joao-Dionisio Could you have a loook about this problem?
SumExpr.coefshas a bug, so this CI failed.If
SumExpr.coefsis not[1, 1], the result fromexpr_to_arraytoaddConswon't be right.All
GenExprwill use theexpr_to_arrayfunction to add toSCIP. Butexpr_to_arraydoesn't have any code to handleSumExpr.coefs.SumExpr.coefsdoesn't work at all.PySCIPOpt/src/pyscipopt/expr.pxi
Lines 829 to 849 in bc24757
Two solutions to fix this.
SumExpr.coefs. Any coefficient to aSumExprwill be aProdExpr. And each element coefficient should be 1. We follow current behavior. So,SumExpr.coefsrelated codes could be removed.expr_to_arraysupportSumExpr.coefs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SumExpr.coefshas a bug. So the latest CI is still failing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Zeroto521 what's the status on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SumExpr.coefshas a bug, as I said before. There are two solutions we could fix that. @Joao-DionisioSumExpr.coefs. Any coefficient to aSumExprwill be aProdExpr. And each element coefficient should be 1. We follow current behavior. So,SumExpr.coefsrelated codes could be removed.expr_to_arraysupportSumExpr.coefs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather extend the support of
expr_to_array. A suggestion:expr_to_array: forOperator.add, includeexpr.coefsandexpr.constantin the node tuple instead of appending the constantscip.pxi(theOperator.addbranch): read the actual coefs and constant from the node and pass them toSCIPcreateExprSum.Do you think this might fix things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to drop
SumExpr.coefsnow.When I'm working on
expr_to_array, I find thatSumExpr.coefscan't be printed well. The coefficient should be close to its value. Butchildrenandcoefsboth arelist, they are not a singledict. Although we could usezip, it is not a better way for__repr__.