Deprecated Term.ptrtuple and speed up Term.__eq__#1184
Open
Zeroto521 wants to merge 10 commits intoscipopt:masterfrom
Open
Deprecated Term.ptrtuple and speed up Term.__eq__#1184Zeroto521 wants to merge 10 commits intoscipopt:masterfrom
Term.ptrtuple and speed up Term.__eq__#1184Zeroto521 wants to merge 10 commits intoscipopt:masterfrom
Conversation
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.
Contributor
There was a problem hiding this comment.
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.ptrtupleattribute to reduce memory footprint - Optimized
Term.__eq__with identity checks, hash comparison, and C-level tuple access - Added
Variable.__hash__()method and madeptr()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.
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>
Zeroto521
commented
Feb 4, 2026
src/pyscipopt/scip.pxi
Outdated
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) |
Contributor
Author
There was a problem hiding this comment.
Variable.__hash__ is different from Expr. So it has to add __richcm__ by hand. Otherwise python can't find the comparison method
Zeroto521
commented
Feb 4, 2026
| 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) |
Contributor
Author
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Termclass hasvartupleandptrtuple. Both are variable containers, so it's duplicated.Removing
Term.ptrtuplecould save about 50% memery.Term.__eq__is faster than the master branch when they differ. And having almost the same speed for the sameTerm.