Skip to content

Commit ddcfc9a

Browse files
author
Release Manager
committed
gh-36068: Speed-up matrix construction by ensuring MatrixArgs type MA_ENTRIES_ZERO There are many ways to specify entries when creating a matrix, which are handled in `args.pyx` with `MatrixArgs` class. It has been discussed before why allowing `entries=None` in the matrix creation methods can be beneficial in terms of performance (#11589 , #12020). This input `entries=None` is required to yield the zero matrix. For example: `matrix(QQ, 4, 4)` creates the zero matrix. This corresponds to the type `MA_ENTRIES_ZERO` in `args.pyx`. When one passes a value, e.g. `matrix(QQ, 4, 4, 2)`, then one gets a diagonal matrix with `2` on the diagonal; this corresponds to the type `MA_ENTRIES_SCALAR` in `args.pyx`. Currently, doing something like `matrix(QQ, 4, 4, 0)` will pick `MA_ENTRIES_SCALAR`, and therefore will build the matrix and fill the diagonal with zero. [Behind the scenes, there is still some acknowledgement that this is not the usual scalar matrix case, since this will not fail if the matrix is not square (`matrix(QQ, 4, 5, 0)` will not fail, but `matrix(QQ, 4, 5, 1)` will). But this is still not seen as `MA_ENTRIES_ZERO`.] This PR ensures the type `MA_ENTRIES_ZERO` is picked in this situation. This improves performance and solves some issues, as noted below. This PR also fixes the related issue #36065 . In fact, `entries=None` is the default in the `__init__` of all matrix subclasses presently implemented. It also used to be the default when constructing a matrix by "calling" a matrix space, until #31078 and https://github.com/sag emath/sage/commit/cef613a0a57b85c1ebc5747185213ae4f5ec35f2 which changed this default value from `None` to `0`, bringing some performance regression. Regarding this "calling the matrix space", this PR also solves the performance issue noted in #35961 (comment) , where it was observed that in the following piece of code: ``` sage: space = MatrixSpace(GF(9001), 10000, 10000) sage: %timeit zmat = space.matrix() 18.3 µs ± 130 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) sage: %timeit zmat = space() 12.1 ms ± 65.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) ``` the called default is not the same. `space.matrix()` directly initializes the matrix through `entries=None`, but on the other hand `space()` actually calls the constructor with `entries=0`. This performance regression comes from #31078 and https://github.com/sag emath/sage/commit/cef613a0a57b85c1ebc5747185213ae4f5ec35f2 , where the default for construction from the matrix space was changed from `None` to `0`. This cannot be easily reverted: this is now the default in the `__call__` of the `Parent` class. So this PR does not directly revert the call default to `None` somehow, but the result is very close in effect (read: the overhead is small). Unchanged: through the parent call, `0` is converted to the base ring zero, which is passed to the constructor's `entries` which is then analyzed as `MA_ENTRIES_SCALAR`. Changed: the code modifications ensure that soon enough it will be detected that this is in fact `MA_ENTRIES_ZERO`. The small overhead explains why, despite the improvements, construction with `None` is still sometimes slightly faster than with `0`. Below are some timings showing the improvements for some fields. Also, this PR merged with the improvements discussed in #35961 will make the above timings of `space.matrix()` and `space()` be the same (which means a speed-up of a factor >500 for this call `space()`...). The measurement is for construction via calling the matrix space: `None` is `space(None)`, `Empty` is `space()`, `Zero` is `space(0)`, and `Nonzero` is `space(some nonzero value)`. ``` NEW TIMES field dim None Empty Zero Nonzero GF(2) 5 2.3e-06 3.2e-06 3.6e-06 3.2e-06 GF(2) 50 2.4e-06 3.3e-06 3.6e-06 5.8e-06 GF(2) 500 3.6e-06 4.5e-06 4.8e-06 3.1e-05 GF(512) 5 2.6e-05 2.8e-05 2.9e-05 2.9e-05 GF(512) 50 2.6e-05 2.9e-05 2.9e-05 4.0e-05 GF(512) 500 3.7e-05 3.8e-05 3.9e-05 1.6e-04 QQ 5 2.2e-06 3.3e-06 3.4e-06 3.2e-06 QQ 50 8.0e-06 9.2e-06 9.4e-06 1.2e-05 QQ 500 6.1e-04 6.3e-04 6.4e-04 6.7e-04 OLD TIMES field dim None Empty Zero Nonzero GF(2) 5 2.3e-06 3.5e-06 3.6e-06 3.7e-06 GF(2) 50 2.4e-06 6.0e-06 6.1e-06 6.0e-06 GF(2) 500 3.6e-06 3.0e-05 3.0e-05 3.0e-05 GF(512) 5 2.5e-05 2.8e-05 2.9e-05 2.9e-05 GF(512) 50 2.5e-05 3.9e-05 4.0e-05 4.0e-05 GF(512) 500 3.5e-05 1.5e-04 1.5e-04 1.6e-04 QQ 5 2.2e-06 3.5e-06 3.7e-06 3.7e-06 QQ 50 7.9e-06 1.2e-05 1.2e-05 1.2e-05 QQ 500 6.4e-04 6.9e-04 6.9e-04 6.9e-04 ``` Code used for the timings: ``` time_kwds = dict(seconds=True, number=20000, repeat=7) fields = [GF(2), GF(2**9), QQ] names = ["GF(2)", "GF(512)", "QQ"] vals = [GF(2)(1), GF(2**9).gen(), 5/2] print(f"field\tdim\tNone\tEmpty\tZero\tNonzero") for field,name,val in zip(fields,names,vals): for dim in [5, 50, 500]: space = MatrixSpace(field, dim, dim) tnone = timeit("mat = space(None)", **time_kwds) tempty = timeit("mat = space()", **time_kwds) tzero = timeit("mat = space(0)", **time_kwds) tnonz = timeit("mat = space(1)", **time_kwds) print(f"{name}\t{dim}\t{tnone:.1e}\t{tempty:.1e}\t{tzero:.1e}\t{ tnonz:.1e}") ``` ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies URL: #36068 Reported by: Vincent Neiger Reviewer(s): Matthias Köppe, Vincent Neiger
2 parents caf98de + 7d476ed commit ddcfc9a

File tree

2 files changed

+35
-7
lines changed

2 files changed

+35
-7
lines changed

‎src/sage/matrix/args.pyx

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ from cysignals.signals cimport sig_check
2121
MatrixSpace = None
2222

2323
from sage.rings.integer_ring import ZZ
24+
from sage.rings.integer cimport Integer
2425
from sage.structure.coerce cimport (coercion_model,
2526
is_numpy_type, py_scalar_parent)
2627
from sage.structure.element cimport Element, RingElement, Vector
@@ -847,6 +848,24 @@ cdef class MatrixArgs:
847848
Traceback (most recent call last):
848849
...
849850
ValueError: sequence too short (expected length 6, got 1)
851+
852+
Check github issue #36065:
853+
854+
sage: class MyAlgebraicNumber(sage.rings.qqbar.AlgebraicNumber):
855+
....: def __bool__(self):
856+
....: raise ValueError
857+
sage: matrix(1, 1, MyAlgebraicNumber(0))
858+
[0]
859+
sage: matrix(1, 1, MyAlgebraicNumber(3))
860+
[3]
861+
sage: matrix(1, 2, MyAlgebraicNumber(0))
862+
Traceback (most recent call last):
863+
...
864+
TypeError: scalar matrix must be square if the value cannot be determined to be zero
865+
sage: matrix(1, 2, MyAlgebraicNumber(3))
866+
Traceback (most recent call last):
867+
...
868+
TypeError: scalar matrix must be square if the value cannot be determined to be zero
850869
"""
851870
self.finalize()
852871
return self
@@ -924,11 +943,18 @@ cdef class MatrixArgs:
924943
raise AssertionError(f"nrows={self.nrows} ncols={self.ncols} base={self.base} type={self.typ}")
925944

926945
# Non-zero scalar matrices must be square
946+
# also ensure type is MA_ENTRIES_ZERO for scalar zero matrices
927947
if self.typ == MA_ENTRIES_SCALAR:
928-
if self.nrows != self.ncols:
929-
if self.entries:
930-
raise TypeError("nonzero scalar matrix must be square")
931-
self.typ = MA_ENTRIES_ZERO
948+
try:
949+
if not self.entries:
950+
self.typ = MA_ENTRIES_ZERO
951+
except Exception:
952+
# "not self.entries" has failed, self.entries cannot be determined to be zero
953+
if self.nrows != self.ncols:
954+
raise TypeError("scalar matrix must be square if the value cannot be determined to be zero")
955+
if self.typ == MA_ENTRIES_SCALAR and self.nrows != self.ncols:
956+
# self.typ is still SCALAR -> "not self.entries" has successfully evaluated, to False
957+
raise TypeError("nonzero scalar matrix must be square")
932958

933959
if self.sparse == -1:
934960
self.sparse = (self.typ & MA_FLAG_SPARSE) != 0
@@ -1219,12 +1245,14 @@ cdef class MatrixArgs:
12191245
# hurt to do these first.
12201246
if self.entries is None:
12211247
return MA_ENTRIES_ZERO
1248+
if isinstance(self.entries, (int, float, complex, Integer)):
1249+
if self.entries:
1250+
return MA_ENTRIES_SCALAR
1251+
return MA_ENTRIES_ZERO
12221252
if isinstance(self.entries, (list, tuple)):
12231253
return self.sequence_type()
12241254
if isinstance(self.entries, dict):
12251255
return MA_ENTRIES_MAPPING
1226-
if isinstance(self.entries, (int, float, complex)):
1227-
return MA_ENTRIES_SCALAR
12281256

12291257
# Note: some objects are callable, iterable and act like a
12301258
# scalar, e.g. polynomials. So the order of these checks

‎src/sage/rings/padics/relaxed_template.pxi

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -811,7 +811,7 @@ cdef class RelaxedElement(pAdicGenericElement):
811811

812812
def __bool__(self):
813813
r"""
814-
Return ``True`` if this element is indistinguishable from zero.
814+
Return ``True`` if this element is distinguishable from zero.
815815
816816
TESTS::
817817

0 commit comments

Comments
 (0)