Conversation
Replaces Term.__add__ with Term.__mul__ and updates Expr.__mul__ to use more efficient Cython dict iteration and item access. This improves performance and correctness when multiplying expressions, especially for large term dictionaries.
Replaces the simple concatenation in Term.__mul__ with an efficient merge that maintains variable order based on pointer values. This improves performance and correctness when multiplying Term objects.
Moved the 'Speed up MatrixExpr.sum(axis=...) via quicksum' entry from the Added section to the Changed section for better categorization and clarity.
Added a new entry to the changelog noting the performance improvement for Expr * Expr operations.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the multiplication of Expr objects (polynomial expressions) by using low-level C API calls and introducing a more efficient Term.__mul__ method. According to the benchmarks provided, this results in at least 1.2x speedup for matrix multiplication operations.
Changes:
- Introduced
Term.__mul__method using an efficient merge algorithm for combining sorted variable tuples - Optimized
Expr.__mul__to use C-level Python dict iteration APIs (PyDict_Next, PyDict_GetItem) and skip zero coefficients - Updated CHANGELOG to document the performance improvement
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/pyscipopt/expr.pxi | Implements optimized Term.__mul__ method and refactors Expr.__mul__ to use low-level C APIs for faster dictionary iteration and term multiplication |
| CHANGELOG.md | Adds entry documenting the Expr * Expr performance improvement in the Changed section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Corrects the Term class in scip.pyi to define __mul__ instead of __add__, updating the method signature to accept and return Term objects.
|
Can you please add a test for this change? Then we can merge, I think |
Introduces test_mul to verify correct string representations of multiplied expressions involving variables and constants.
Replaces Term with CONST import from pyscipopt.scip and adds new assertions in test_mul to verify multiplication involving constants and variables. Removes redundant CONST definition.
Added an assertion to test that multiplying y by (x - x) yields the same zero term as (x - x) * y. This ensures correct handling of zero expressions in multiplication.
Documented performance enhancements for Expr * Expr and Term * Term operations, including use of C-level API and an O(n) algorithm. Also clarified method renaming from Term.__add__ to Term.__mul__.
Added a comment in the Term.__mul__ method to highlight that Term.vartuple must be sorted for correct merging. Suggests ensuring sorting in the Term constructor to avoid potential issues.
Updated the description of the Term * Term speedup to specify the use of an O(n) sort algorithm instead of Python's O(log(n)) sorted function.
Updated the changelog to fix the time complexity notation for the Term * Term sort algorithm from O(log(n)) to O(n log(n)).
|
Can you please run the performance experiments a bit more systematically? Maybe with multiple matrices and non-matrices, and show an average across instance size. |
|
|
No sorry, I meant in your local environment, not in the tests. I'm very close to merging, just want a final experiment that's more robust regarding the performance. |
Corrected the indentation of the isinstance(other, Expr) block in the Expr class to ensure proper execution flow during multiplication operations.
|
This PR is 1.2x faster than master.
Two normal from timeit import timeit
from pyscipopt import Model, quickprod
m = Model()
n = 10_000
print(f"create 2 `Expr` which have {n:,} vars each")
x = quickprod(m.addMatrixVar(n).flat)
y = quickprod(m.addMatrixVar(n).flat)
number = 100_000
print(f"repeat {number:,} times")
cost = timeit(lambda: x * y, number=number)
print(f"XXX: {cost:.10f} seconds")Conda Environment# packages in environment at D:\Users\Zero\AppData\Local\miniforge3\envs\cy310:
#
# Name Version Build Channel
bzip2 1.0.8 h0ad9c76_8 conda-forge
ca-certificates 2025.11.12 h4c7d964_0 conda-forge
certifi 2025.11.12 pypi_0 pypi
charset-normalizer 3.4.4 pypi_0 pypi
colorama 0.4.6 pyhd8ed1ab_1 conda-forge
cython 3.2.3 py310h23e71ea_0 conda-forge
exceptiongroup 1.3.1 pyhd8ed1ab_0 conda-forge
execnet 2.1.2 pyhd8ed1ab_0 conda-forge
idna 3.11 pypi_0 pypi
iniconfig 2.3.0 pyhd8ed1ab_0 conda-forge
libblas 3.9.0 35_h5709861_mkl conda-forge
libcblas 3.9.0 35_h2a3cdd5_mkl conda-forge
libexpat 2.7.3 hac47afa_0 conda-forge
libffi 3.5.2 h52bdfb6_0 conda-forge
libhwloc 2.11.2 default_hc8275d1_1000 conda-forge
libiconv 1.18 hc1393d2_2 conda-forge
liblapack 3.9.0 35_hf9ab0e9_mkl conda-forge
liblzma 5.8.1 h2466b09_2 conda-forge
libpython 0.2 pypi_0 pypi
libpython-static 3.10.19 hac47afa_2_cpython conda-forge
libsqlite 3.51.1 hf5d6505_1 conda-forge
libxml2 2.13.9 h741aa76_0 conda-forge
libzlib 1.3.1 h2466b09_2 conda-forge
llvm-openmp 21.1.8 h4fa8253_0 conda-forge
m2w64-binutils 2.25.1 5 conda-forge
m2w64-bzip2 1.0.6 6 conda-forge
m2w64-crt-git 5.0.0.4636.2595836 2 conda-forge
m2w64-gcc 5.3.0 6 conda-forge
m2w64-gcc-ada 5.3.0 6 conda-forge
m2w64-gcc-fortran 5.3.0 6 conda-forge
m2w64-gcc-libgfortran 5.3.0 6 conda-forge
m2w64-gcc-libs 5.3.0 7 conda-forge
m2w64-gcc-libs-core 5.3.0 7 conda-forge
m2w64-gcc-objc 5.3.0 6 conda-forge
m2w64-gmp 6.1.0 2 conda-forge
m2w64-headers-git 5.0.0.4636.c0ad18a 2 conda-forge
m2w64-isl 0.16.1 2 conda-forge
m2w64-libiconv 1.14 6 conda-forge
m2w64-libmangle-git 5.0.0.4509.2e5a9a2 2 conda-forge
m2w64-libwinpthread-git 5.0.0.4634.697f757 2 conda-forge
m2w64-make 4.1.2351.a80a8b8 2 conda-forge
m2w64-mpc 1.0.3 3 conda-forge
m2w64-mpfr 3.1.4 4 conda-forge
m2w64-pkg-config 0.29.1 2 conda-forge
m2w64-toolchain 5.3.0 7 conda-forge
m2w64-tools-git 5.0.0.4592.90b8472 2 conda-forge
m2w64-windows-default-manifest 6.4 3 conda-forge
m2w64-winpthreads-git 5.0.0.4634.697f757 2 conda-forge
m2w64-zlib 1.2.8 10 conda-forge
mkl 2024.2.2 h57928b3_16 conda-forge
msys2-conda-epoch 20160418 1 conda-forge
numpy 2.2.6 py310h4987827_0 conda-forge
openssl 3.6.0 h725018a_0 conda-forge
packaging 25.0 pyh29332c3_1 conda-forge
pip 25.3 pyh8b19718_0 conda-forge
pluggy 1.6.0 pyhf9edf01_1 conda-forge
pthreads-win32 2.9.1 h2466b09_4 conda-forge
pygments 2.19.2 pyhd8ed1ab_0 conda-forge
pytest 9.0.2 pyhcf101f3_0 conda-forge
pytest-xdist 3.8.0 pyhd8ed1ab_0 conda-forge
python 3.10.19 hc20f281_2_cpython conda-forge
python_abi 3.10 8_cp310 conda-forge
requests 2.32.5 pypi_0 pypi
setuptools 80.9.0 pyhff2d567_0 conda-forge
tbb 2021.13.0 h62715c5_1 conda-forge
tk 8.6.13 h2c6b04d_3 conda-forge
tomli 2.3.0 pyhcf101f3_0 conda-forge
typing_extensions 4.15.0 pyhcf101f3_0 conda-forge
tzdata 2025c h8577fbf_0 conda-forge
ucrt 10.0.26100.0 h57928b3_0 conda-forge
urllib3 2.6.2 pypi_0 pypi
vc 14.3 h2b53caa_33 conda-forge
vc14_runtime 14.44.35208 h818238b_33 conda-forge
vcomp14 14.44.35208 h818238b_33 conda-forge
wheel 0.45.1 pyhd8ed1ab_1 conda-forge |
Do not skip terms with 0.0 coefficients when multiplying Expr objects: remove earlier zero-checks and compute product values inline in src/pyscipopt/expr.pxi. This causes zero-product terms to be retained in the resulting expression. Update tests (tests/test_expr.py) to expect the preserved zero-coefficient terms for cases like (x - x) * y and y * (x - x).
This PR is at least faster 1.2x than before.
Expr.__mul__(Expr):Expr.__mul__(Expr)andTerm.__mul__(Term)