Feature: Speed up MatrixVariable.sum(axis=None) via quicksum
#1078
Conversation
Added a test to verify that summing a matrix variable with axis=0 returns a MatrixExpr with the expected shape. This improves coverage for matrix sum operations with axis specified.
Introduced a new test to compare performance of matrix sum versus element-wise sum. Refactored imports for clarity and consistency. Renamed performance test for better description.
| import pdb | ||
| import pprint |
There was a problem hiding this comment.
Remove unused importing
| from pyscipopt import ( | ||
| Expr, | ||
| ExprCons, | ||
| MatrixConstraint, | ||
| MatrixExpr, | ||
| MatrixExprCons, | ||
| MatrixVariable, | ||
| Model, | ||
| Variable, | ||
| cos, | ||
| exp, | ||
| log, | ||
| quicksum, | ||
| sin, | ||
| sqrt, | ||
| ) |
| Based on `numpy.ndarray.sum`, but returns a scalar if `axis=None`. | ||
| This is useful for matrix expressions to compare with a matrix or a scalar. |
There was a problem hiding this comment.
An example to show this change. This behavior is similar to numpy.ndarray.sum.
model = Model()
x = model.addMatrixVar((3, 1))
y = model.addMatrixVar(1)
# Now it can directly compare.
# Before, it would raise an error because of different shapes.
x.sum(axis=0) == y
There was a problem hiding this comment.
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 scalar
Renamed the Model instance from 'm' to 'model' to avoid confusion with the integer variable 'm' and improve code clarity in the test_sum_performance function.
Replaces incorrect usage of 'm' with 'model' in the assertion within test_sum_performance to ensure the correct object is referenced.
Updated the assertion in test_matrix_sum_argument to expect shape (1,) instead of (1, 1) when summing along axis 0. This aligns the test with the actual output of the sum operation.
|
Do you mind extending the performance tests for matrix variables and reporting the speed improvement this PR brings please? And thank you very much, as always! |
|
In my local environment, this change is 50x faster than before for the |
Sorry, I didn't get it. |
Modified the assertion in test_sum_performance to compare orig_time + 1 with matrix_time instead of orig_time. This may address timing precision or test flakiness.
|
Oh sorry @Zeroto521, I hadn't looked at the changes yet, thank you! Can you think of other things that people using matrix variables would want to be sped up? Things like adding big matrix variables, etc. If so, would you mind adding such performance tests? That's what I meant, if you think it's a reasonable request. Great work with all your PRs, by the way! 💪 |
|
|
||
| # Original sum via `np.sum` | ||
| start_orig = time() | ||
| np.sum(x) |
There was a problem hiding this comment.
Did you want np.sum, or the previous behavior of x.sum?
2 participants
MatrixVariable.sum(axis=None)viaquicksumMatrixVariable.sumis slower thanquicksum#1070.axis=None.