Skip to content

Deprecated Term.ptrtuple and speed up Term.__eq__#1184

Open
Zeroto521 wants to merge 10 commits intoscipopt:masterfrom
Zeroto521:Term/ptr
Open

Deprecated Term.ptrtuple and speed up Term.__eq__#1184
Zeroto521 wants to merge 10 commits intoscipopt:masterfrom
Zeroto521:Term/ptr

Conversation

@Zeroto521
Copy link
Contributor

@Zeroto521 Zeroto521 commented Feb 4, 2026

Term class has vartuple and ptrtuple. Both are variable containers, so it's duplicated.
Removing Term.ptrtuple could save about 50% memery.
Term.__eq__ is faster than the master branch when they differ. And having almost the same speed for the same Term.

Test item Optimized before Optimized after
Memery usage 15.77KB 7.91KB
Same Terms to compare 0.9245s 1.0219s
Different Terms to compare 0.3283s 0.0066s
from timeit import timeit
import sys

from pyscipopt import Model, quickprod


def memsize(obj):
    size_bytes = sys.getsizeof(obj) + sys.getsizeof(obj.vartuple)
    if hasattr(obj, "ptrtuple"):
        size_bytes += sys.getsizeof(obj.ptrtuple)

    if size_bytes < 1024:
        return f"{size_bytes}B"
    elif size_bytes < 1024**2:
        return f"{size_bytes / 1024:.2f}KB"
    return f"{size_bytes / 1024**2:.2f}MB"


m = Model()

n = 1_000
print(f"create 2 `Term` which have {n:,} vars each")
x = m.addMatrixVar(n)
e1 = quickprod(x.flat)
e2 = quickprod(x.flat)
t1 = next(iter(e1))
t2 = next(iter(e2))
print(t1 == t2)  # should be True
print(f"memory size: {memsize(t1)}")

y = m.addVar(name="y")
z = m.addVar(name="z")
t3 = next(iter(e1 * y))
t4 = next(iter(e2 * z))
print(t3 == t4)  # should be False

number = 100_000
print(f"repeat {number:,} times")
cost = timeit(lambda: t1 == t2, number=number)
print(f"XXX: {cost:.4f} seconds")

cost = timeit(lambda: t3 == t4, number=number)
print(f"XXX: {cost:.4f} seconds")

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).
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.
Copilot AI review requested due to automatic review settings February 4, 2026 05:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request optimizes the Term class by removing the ptrtuple attribute and reimplementing __eq__ using C-level APIs. The changes aim to reduce memory usage by ~50% and improve comparison performance, particularly for different terms.

Changes:

  • Removed Term.ptrtuple attribute to reduce memory footprint
  • Optimized Term.__eq__ with identity checks, hash comparison, and C-level tuple access
  • Added Variable.__hash__() method and made ptr() delegate to it for consistency

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/pyscipopt/scip.pyi Removed ptrtuple attribute declaration from Term class type stub
src/pyscipopt/scip.pxi Added __hash__() method to Variable class and refactored ptr() to call it
src/pyscipopt/expr.pxi Removed ptrtuple attribute, replaced pointer-based sorting/hashing with hash-based approach, and optimized __eq__ implementation
CHANGELOG.md Documented the removal of ptrtuple and performance improvements to __eq__

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Zeroto521 and others added 4 commits February 4, 2026 13:44
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.
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.
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Comment on lines 1564 to 1568
def ptr(self) -> size_t:
return self.__hash__()

def __richcmp__(self, other, int op):
return _expr_richcmp(self, other, op)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable.__hash__ is different from Expr. So it has to add __richcm__ by hand. Otherwise python can't find the comparison method

res.vartuple = tuple(vartuple)
res.ptrtuple = tuple(v.ptr() for v in res.vartuple)
res.hashval = <Py_ssize_t>hash(res.ptrtuple)
res.hashval = <Py_ssize_t>hash(res.vartuple)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hash(res.vartuple) reqires Variable.__hash__.

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.
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.
warning: src\pyscipopt\scip.pxi:1564:21: Unknown type declaration 'Py_ssize_t' in annotation, ignoring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant