Skip to content

DEP: remove _is_number(Expr) from expr.pyi file #1168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Zeroto521 wants to merge 5 commits into
base: master
Choose a base branch
from
Open

DEP: remove _is_number(Expr) from expr.pyi file #1168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57b2861
Refactor Expr arithmetic methods to simplify logic
Zeroto521 Jan 22, 2026
582283f
Merge branch 'master' into dep/_is_number(self)
Zeroto521 Jan 22, 2026
532dee4
Merge branch 'master' into dep/_is_number(self)
Joao-Dionisio Jan 22, 2026
1d0e6f5
Refactor performance tests to use timeit and update assertions
Zeroto521 Jan 23, 2026
f67ec3b
use a fixed value for constant
Zeroto521 Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions src/pyscipopt/expr.pxi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,6 @@ cdef class Expr:
def __add__(self, other):
left = self
right = other

if _is_number(self):
assert isinstance(other, Expr)
left,right = right,left
terms = left.terms.copy()

if isinstance(right, Expr):
Expand Down Expand Up @@ -254,9 +250,6 @@ cdef class Expr:
if _is_number(other):
f = float(other)
return Expr({v:f*c for v,c in self.terms.items()})
elif _is_number(self):
f = float(self)
return Expr({v:f*c for v,c in other.terms.items()})
elif isinstance(other, Expr):
terms = {}
for v1, c1 in self.terms.items():
Expand All @@ -278,11 +271,7 @@ cdef class Expr:

def __rtruediv__(self, other):
''' other / self '''
if _is_number(self):
f = 1.0/float(self)
return f * other
otherexpr = buildGenExprObj(other)
return otherexpr.__truediv__(self)
return buildGenExprObj(other) / self

def __pow__(self, other, modulo):
if float(other).is_integer() and other >= 0:
Expand Down
63 changes: 24 additions & 39 deletions tests/test_matrix_variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import operator
from time import time
from timeit import timeit

import numpy as np
import pytest
Expand Down Expand Up @@ -257,58 +258,46 @@ def test_matrix_sum_result(axis, keepdims):
assert np_res.shape == scip_res.shape


@pytest.mark.parametrize("n", [50, 100])
Copy link
Contributor Author

@Zeroto521 Zeroto521 Jan 23, 2026

Choose a reason for hiding this comment

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

A small data size will fail sometimes

@pytest.mark.parametrize("n", [100])
def test_matrix_sum_axis_is_none_performance(n):
model = Model()
x = model.addMatrixVar((n, n))

# Original sum via `np.ndarray.sum`
start = time()
x.view(np.ndarray).sum()
orig = time() - start

number = 5
# Optimized sum via `quicksum`
start = time()
x.sum()
matrix = time() - start
matrix = timeit(lambda: x.sum(), number=number) / number
# Original sum via `np.ndarray.sum`
orig = timeit(lambda: x.view(np.ndarray).sum(), number=number) / number

assert model.isGT(orig, matrix)
assert model.isGE(orig * 1.25, matrix)


@pytest.mark.parametrize("n", [50, 100])
@pytest.mark.parametrize("n", [100])
def test_matrix_sum_axis_not_none_performance(n):
model = Model()
x = model.addMatrixVar((n, n))

# Original sum via `np.ndarray.sum`
start = time()
x.view(np.ndarray).sum(axis=0)
orig = time() - start

number = 5
# Optimized sum via `quicksum`
start = time()
x.sum(axis=0)
matrix = time() - start
matrix = timeit(lambda: x.sum(axis=0), number=number) / number
# Original sum via `np.ndarray.sum`
orig = timeit(lambda: x.view(np.ndarray).sum(axis=0), number=number) / number

assert model.isGT(orig, matrix)
assert model.isGE(orig * 1.25, matrix)


@pytest.mark.parametrize("n", [50, 100])
@pytest.mark.parametrize("n", [100])
def test_matrix_mean_performance(n):
model = Model()
x = model.addMatrixVar((n, n))

number = 5
# Original mean via `np.ndarray.mean`
start = time()
x.view(np.ndarray).mean(axis=0)
orig = time() - start

matrix = timeit(lambda: x.mean(axis=0), number=number) / number
# Optimized mean via `quicksum`
start = time()
x.mean(axis=0)
matrix = time() - start
orig = timeit(lambda: x.view(np.ndarray).mean(axis=0), number=number) / number

assert model.isGT(orig, matrix)
assert model.isGE(orig * 1.25, matrix)


def test_matrix_mean():
Expand All @@ -319,21 +308,17 @@ def test_matrix_mean():
assert isinstance(x.mean(1), MatrixExpr)


@pytest.mark.parametrize("n", [50, 100])
@pytest.mark.parametrize("n", [100])
def test_matrix_dot_performance(n):
model = Model()
x = model.addMatrixVar((n, n))
a = np.random.rand(n, n)

start = time()
a @ x.view(np.ndarray)
orig = time() - start
a = np.vstack((np.zeros((n // 2, n)), np.ones((n // 2, n))))
Copy link
Contributor Author

@Zeroto521 Zeroto521 Jan 23, 2026

Choose a reason for hiding this comment

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

Use a fixed constant matrix (a half-zeroes and a half-ones) instead of a random matrix.


start = time()
a @ x
matrix = time() - start
number = 5
matrix = timeit(lambda: a @ x, number=number) / number
Copy link
Contributor Author

@Zeroto521 Zeroto521 Jan 23, 2026

Choose a reason for hiding this comment

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

Use timeit avoid pytest using cache

Copy link
Contributor Author

@Zeroto521 Zeroto521 Jan 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

But I have to say, performance test cases sometimes can't pass.
All of this (use timeit, isGE, and *1.25) is decreasing the possibility.
I tested 20 times for pytest -n auto tests/test_matrix_variable.py, and 1 time failed.

Copy link
Member

@Joao-Dionisio Joao-Dionisio Jan 23, 2026

Choose a reason for hiding this comment

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

Yes, there is some performance variability, but the goal of using matrices and a significant portion of your PRs is to increase performance over not using them. Agreed that it makes sense to reduce randomness in the performance tests.

orig = timeit(lambda: a @ x.view(np.ndarray), number=number) / number

assert model.isGT(orig, matrix)
assert model.isGE(orig * 1.25, matrix)


def test_matrix_dot_value():
Expand Down
Loading