Skip to content

Commit 5329bc1

Browse files
Zeroto521CopilotJoao-Dionisio
authored
Deprecated Term.ptrtuple and speed up Term.__eq__ (#1184)
* Deprecated `Term.ptrtuple` Remove the separate ptrtuple from Term and switch sorting/merging to use each Variable's hash instead of a ptr() tuple. Compute Term.hashval from vartuple directly and implement a robust __eq__ that checks identity, type, length, hashval and then element-wise variable identity. In Variable, expose __hash__ returning the underlying scip_var pointer and make ptr() delegate to that hash. Update the .pyi stub to drop the now-removed ptrtuple. These changes simplify Term state and unify ordering/identity on Variable hashes (pointer-based). * Changelog: Term __eq__ speedup, ptrtuple deprecated Update CHANGELOG.md to record a performance improvement: Term.__eq__ was sped up using the C-level API, and to note that Term.ptrtuple is deprecated to enable Term memory optimizations. * Add rich comparison to Variable Implement __richcmp__ in Variable (src/pyscipopt/scip.pxi) to delegate rich-comparison operations to _expr_richcmp, enabling Python comparison operators between Variable and other Expr objects. * tests: add term equality tests for expressions Import quickprod and add test_term_eq to verify term equality/inequality behavior. The new test creates terms from quickprod over a matrix variable and checks that identical terms compare equal, different terms (even of same length) compare unequal, terms of different lengths are unequal, and comparison with a different type returns False. This ensures Expr term __eq__ semantics are correct. * Use hash comparison for variable equality in Term Replace identity check with hash comparison when comparing variables in Term equality. Using hash(var) != hash(var) instead of `is not` avoids false negatives when two distinct Python wrapper objects represent the same underlying variable, ensuring Term comparisons treat equivalent variables as equal. This relies on Variable.__hash__ to reflect variable identity. * Apply AI suggestion Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Improve Term.__eq__ type check Remove the strict Cython annotation on the __eq__ parameter and replace the Python-level type() check with a C-level Py_TYPE check. Also normalize the identity check order. These changes make the equality method more robust and slightly faster by avoiding a Python call for type comparison and allowing broader input types during runtime. * Promote Term changes to top of CHANGELOG Move two entries about Term optimizations out of the 6.1.0 section and into the top-level changelog sections: "Speed up Term.__eq__ via the C-level API" (Changed) and "Removed Term.ptrtuple to optimize Term memory usage" (Removed). This removes duplicate lines from 6.1.0 and surfaces these changes for the upcoming release. * Remove return type annotations in scip.pxi warning: src\pyscipopt\scip.pxi:1564:21: Unknown type declaration 'Py_ssize_t' in annotation, ignoring * Drop `Variable.__hash__` * Hash Term by tuple of variable pointers hash(iteration) isn't fixed value --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: João Dionísio <57299939+Joao-Dionisio@users.noreply.github.com>
1 parent 9271db1 commit 5329bc1

5 files changed

Lines changed: 45 additions & 10 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,10 @@
1010
- Fixed stubtest failures with mypy 1.20 by marking dunder method parameters as positional-only
1111
### Changed
1212
- Speed up `constant * Expr` via C-level API
13+
- Speed up `Term.__eq__` via the C-level API
1314
### Removed
1415
- Removed outdated warning about Make build system incompatibility
16+
- Removed `Term.ptrtuple` to optimize `Term` memory usage
1517

1618
## 6.1.0 - 2026.01.31
1719
### Added

src/pyscipopt/expr.pxi

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -104,22 +104,37 @@ cdef class Term:
104104
'''This is a monomial term'''
105105

106106
cdef readonly tuple vartuple
107-
cdef readonly tuple ptrtuple
108107
cdef Py_ssize_t hashval
109108

110109
def __init__(self, *vartuple: Variable):
111110
self.vartuple = tuple(sorted(vartuple, key=lambda v: v.getIndex()))
112-
self.ptrtuple = tuple(v.ptr() for v in self.vartuple)
113-
self.hashval = <Py_ssize_t>hash(self.ptrtuple)
111+
self.hashval = <Py_ssize_t>hash(tuple(v.ptr() for v in self.vartuple))
114112

115113
def __getitem__(self, idx):
116114
return self.vartuple[idx]
117115

118116
def __hash__(self) -> Py_ssize_t:
119117
return self.hashval
120118

121-
def __eq__(self, other: Term):
122-
return self.ptrtuple == other.ptrtuple
119+
def __eq__(self, other) -> bool:
120+
if other is self:
121+
return True
122+
if <type>Py_TYPE(other) is not Term:
123+
return False
124+
125+
cdef int n = len(self)
126+
cdef Term _other = <Term>other
127+
if n != len(_other) or self.hashval != _other.hashval:
128+
return False
129+
130+
cdef int i
131+
cdef Variable var1, var2
132+
for i in range(n):
133+
var1 = <Variable>PyTuple_GET_ITEM(self.vartuple, i)
134+
var2 = <Variable>PyTuple_GET_ITEM(_other.vartuple, i)
135+
if var1.ptr() != var2.ptr():
136+
return False
137+
return True
123138

124139
def __len__(self):
125140
return len(self.vartuple)
@@ -156,8 +171,7 @@ cdef class Term:
156171

157172
cdef Term res = Term.__new__(Term)
158173
res.vartuple = tuple(vartuple)
159-
res.ptrtuple = tuple(v.ptr() for v in res.vartuple)
160-
res.hashval = <Py_ssize_t>hash(res.ptrtuple)
174+
res.hashval = <Py_ssize_t>hash(tuple(v.ptr() for v in res.vartuple))
161175
return res
162176

163177
def __repr__(self):

src/pyscipopt/scip.pxi

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1565,7 +1565,6 @@ cdef class Variable(Expr):
15651565
return cname.decode('utf-8')
15661566

15671567
def ptr(self):
1568-
""" """
15691568
return <size_t>(self.scip_var)
15701569

15711570
def __repr__(self):

src/pyscipopt/scip.pyi

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2202,7 +2202,6 @@ class SumExpr(GenExpr):
22022202

22032203
@disjoint_base
22042204
class Term:
2205-
ptrtuple: Incomplete
22062205
vartuple: Incomplete
22072206
def __init__(self, *vartuple: Incomplete) -> None: ...
22082207
def __mul__(self, other: Term, /) -> Term: ...

tests/test_expr.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import pytest
44

5-
from pyscipopt import Model, sqrt, log, exp, sin, cos
5+
from pyscipopt import Model, sqrt, log, exp, sin, cos, quickprod
66
from pyscipopt.scip import Expr, GenExpr, ExprCons, CONST
77

88

@@ -249,3 +249,24 @@ def test_abs_abs_expr():
249249

250250
# should print abs(x) not abs(abs(x))
251251
assert str(abs(abs(x))) == str(abs(x))
252+
253+
254+
def test_term_eq():
255+
m = Model()
256+
257+
x = m.addMatrixVar(1000)
258+
y = m.addVar()
259+
z = m.addVar()
260+
261+
e1 = quickprod(x.flat)
262+
e2 = quickprod(x.flat)
263+
t1 = next(iter(e1))
264+
t2 = next(iter(e2))
265+
t3 = next(iter(e1 * y))
266+
t4 = next(iter(e2 * z))
267+
268+
assert t1 == t1 # same term
269+
assert t1 == t2 # same term
270+
assert t3 != t4 # same length, but different term
271+
assert t1 != t3 # different length
272+
assert t1 != "not a term" # different type

0 commit comments

Comments
 (0)