Speed up Expr __add__ and __iadd__#1205
Conversation
Refactor Expr.__add__ and __iadd__ to centralize term merging in a new C helper _to_dict. _to_dict iterates other.terms with PyDict_Next, skips zero coefficients, and either copies or mutates the target dict depending on the copy flag. __add__ now handles numeric additions first and uses _to_dict(copy=True) for Expr+Expr; __iadd__ uses _to_dict(copy=False) to perform in-place merges and returns self. GenExpr and numpy-array cases are preserved. Error messages were slightly adjusted and numeric values are cast to double for correctness and performance.
Add unit tests to verify Expr + Expr and Expr += Expr behavior. test_Expr_add_Expr constructs -x+1 and y-1, checks their string representations and the combined result (including a 0.0 constant term). test_Expr_iadd_Expr verifies in-place addition mutates the left expression, preserves the right expression, and checks their string representations.
There was a problem hiding this comment.
Pull request overview
This PR optimizes Expr.__add__ / Expr.__iadd__ by moving term merging into a C-level helper (_to_dict) that iterates dictionaries via PyDict_Next, aiming to reduce Python-level overhead in expression addition.
Changes:
- Reworked
Expr.__add__andExpr.__iadd__to use a new C-level merge helper forExpr + Expr/Expr += Expr. - Added unit tests covering
Expr + ExprandExpr += Exprbasic behavior. - Documented the optimization in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/pyscipopt/expr.pxi |
Implements faster term merging for addition/in-place addition via _to_dict and updates the dunder methods to use it. |
tests/test_expr.py |
Adds tests validating string representations after Expr + Expr and Expr += Expr. |
CHANGELOG.md |
Notes the new optimization in the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __add__(self, other): | ||
| left = self | ||
| right = other | ||
| terms = left.terms.copy() | ||
|
|
||
| if isinstance(right, Expr): | ||
| # merge the terms by component-wise addition | ||
| for v,c in right.terms.items(): | ||
| terms[v] = terms.get(v, 0.0) + c | ||
| elif _is_number(right): | ||
| c = float(right) | ||
| terms[CONST] = terms.get(CONST, 0.0) + c | ||
| elif isinstance(right, GenExpr): | ||
| return buildGenExprObj(left) + right | ||
| elif isinstance(right, np.ndarray): | ||
| return right + left | ||
| if _is_number(other): | ||
| terms = self.terms.copy() | ||
| terms[CONST] = terms.get(CONST, 0.0) + <double>other | ||
| return Expr(terms) | ||
| elif isinstance(other, Expr): | ||
| return Expr(_to_dict(self, other, copy=True)) | ||
| elif isinstance(other, GenExpr): |
There was a problem hiding this comment.
__add__ now checks _is_number(other) before isinstance(other, Expr). Since _is_number calls float(e) and relies on exceptions for non-numeric types, adding two Expr objects will raise/catch TypeError on every call, which is avoidable overhead in the hot path. Consider checking Expr/GenExpr/np.ndarray first (or replacing _is_number with a cheaper non-exception-based numeric check) and only then falling back to number handling.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1205 +/- ##
==========================================
+ Coverage 57.17% 57.24% +0.07%
==========================================
Files 26 26
Lines 5674 5707 +33
==========================================
+ Hits 3244 3267 +23
- Misses 2430 2440 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pr is 1.75x faster than the master branch.