-
Notifications
You must be signed in to change notification settings - Fork 271
Feature: Speed up MatrixVariable.sum(axis=None)
via quicksum
#1078
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
5437dfd
061eaac
5204d78
c425ebc
7b799b0
929fc1a
6fb8c0d
da6eadb
462f669
44fd2ff
f6ae036
9e52c45
0e53831
5a8cb87
1e18dec
2782693
6c95c94
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,24 @@ | ||
import pdb | ||
import pprint | ||
Comment on lines
-1
to
-2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unused importing |
||
import pytest | ||
from pyscipopt import Model, Variable, log, exp, cos, sin, sqrt | ||
from pyscipopt import Expr, MatrixExpr, MatrixVariable, MatrixExprCons, MatrixConstraint, ExprCons | ||
from time import time | ||
|
||
import numpy as np | ||
import pytest | ||
|
||
from pyscipopt import ( | ||
Expr, | ||
ExprCons, | ||
MatrixConstraint, | ||
MatrixExpr, | ||
MatrixExprCons, | ||
MatrixVariable, | ||
Model, | ||
Variable, | ||
cos, | ||
exp, | ||
log, | ||
quicksum, | ||
sin, | ||
sqrt, | ||
) | ||
Comment on lines
+6
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lint via ruff |
||
|
||
|
||
def test_catching_errors(): | ||
|
@@ -170,6 +183,10 @@ def test_expr_from_matrix_vars(): | |
def test_matrix_sum_argument(): | ||
m = Model() | ||
|
||
# Return a array when axis isn't None | ||
res = m.addMatrixVar((3, 1)).sum(axis=0) | ||
assert isinstance(res, MatrixExpr) and res.shape == (1,) | ||
|
||
# compare the result of summing 2d array to a scalar with a scalar | ||
x = m.addMatrixVar((2, 3), "x", "I", ub=4) | ||
m.addMatrixCons(x.sum() == 24) | ||
|
@@ -192,6 +209,25 @@ def test_matrix_sum_argument(): | |
assert (m.getVal(x) == np.full((2, 3), 4)).all().all() | ||
assert (m.getVal(y) == np.full((2, 4), 3)).all().all() | ||
|
||
|
||
def test_sum_performance(): | ||
n = 1000 | ||
model = Model() | ||
x = model.addMatrixVar((n, n)) | ||
|
||
# Original sum via `np.sum` | ||
start_orig = time() | ||
np.sum(x) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
end_orig = time() | ||
|
||
# Optimized sum via `quicksum` | ||
start_matrix = time() | ||
x.sum() | ||
end_matrix = time() | ||
|
||
assert model.isGT(end_orig - start_orig, end_matrix - start_matrix) | ||
|
||
|
||
def test_add_cons_matrixVar(): | ||
m = Model() | ||
matrix_variable = m.addMatrixVar(shape=(3, 3), vtype="B", name="A", obj=1) | ||
|
@@ -339,7 +375,7 @@ def test_MatrixVariable_attributes(): | |
assert x.varMayRound().tolist() == [[True, True], [True, True]] | ||
|
||
@pytest.mark.skip(reason="Performance test") | ||
def test_performance(): | ||
def test_add_cons_performance(): | ||
start_orig = time() | ||
m = Model() | ||
x = {} | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example to show this change. This behavior is similar to
numpy.ndarray.sum
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can you please explain what
axis=None
means? I don't understand how either the LHS or the RHS of the expression are a scalarUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
y isn't a scaler, it is a matrix.
matrix.sum(axis=None)
(=matrix.sum()
) follows the pyscipopt style to get a scalar. So there won't be any damage to the user.The rest follows
np.ndarray.sum
style.np.ndarray.sum(axis=1)
will return a newnp.ndarray
, not a scalar. To pyscipopt, the return ofMatrixVariable.sum(axis=1)
is always a matrix. We don't need to check that the return is a matrix or a scalar. And the result could continuously compare with or calculate against other matrices.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model.addMatrixVar((3, 1)).sum(axis=0)
return a scalar. Because its size is 1.model.addMatrixVar((3, 1)).sum(axis=0)
return a matrix.