Skip to content

Rewrite A.select(A < 7) to a select expression - #252

Merged
eriknw merged 1 commit into
python-graphblas:mainfrom
eriknw:rewrite_select_expr
Jun 1, 2022
Merged

eriknw merged 1 commit into
python-graphblas:mainfrom
eriknw:rewrite_select_expr

Conversation

@eriknw

@eriknw eriknw commented Jun 1, 2022

Copy link
Copy Markdown
Member

Straightforward pattern-match and rewrite. This is an optimization of (4) from #234 and #251.

@eriknw
eriknw requested a review from jim22k June 1, 2022 02:12
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.4%) to 99.366% when pulling db3bc31 on eriknw:rewrite_select_expr into ecfcd0c on python-graphblas:main.

@eriknw

eriknw commented Jun 1, 2022

Copy link
Copy Markdown
Member Author

Pattern matching ftw!

@eriknw
eriknw merged commit 5d0b361 into python-graphblas:main Jun 1, 2022
eriknw added a commit that referenced this pull request Nov 3, 2026
…osed one

select._match_expr rewrote A.select(A.T == x) as select.valueeq(A, x):
its clause "args[0] is parent or args[0].T is parent" matched the
transpose of the parent, and the rewrite then compared A itself, dropping
the transpose and, for a matrix that is not square, the dimension check:

    S = Matrix.from_coo([0, 1], [1, 0], [1, 5], nrows=2, ncols=2)
    S.select(S.T == 1)
    before: (0, 1) = 1
    after:  (1, 0) = 5, as S.select((S.T == 1).new()) gives
    B (2 x 3): B.select(B.T == 1)
    before: a 2 x 3 result
    after:  DimensionMismatch

The clause is now "args[0] is parent"; anything else takes the mask path.
On main since #252, for built-in dtypes. "Select a UDT's elements equal or
unequal to a value, as == compares them" sent UDT eq and ne through the
same clause, which had made A.select(A.T == x) on a UDT a wrong answer
where it raised before. test_udt_select_by_equality now uses a 2 x 3
matrix with entries off the diagonal, and a mask with replace.
eriknw added a commit that referenced this pull request Nov 13, 2026
* Select by a transposed comparison as by its mask, not by the untransposed one

select._match_expr rewrote A.select(A.T == x) as select.valueeq(A, x):
its clause "args[0] is parent or args[0].T is parent" matched the
transpose of the parent, and the rewrite then compared A itself, dropping
the transpose and, for a matrix that is not square, the dimension check:

    S = Matrix.from_coo([0, 1], [1, 0], [1, 5], nrows=2, ncols=2)
    S.select(S.T == 1)
    before: (0, 1) = 1
    after:  (1, 0) = 5, as S.select((S.T == 1).new()) gives
    B (2 x 3): B.select(B.T == 1)
    before: a 2 x 3 result
    after:  DimensionMismatch

The clause is now "args[0] is parent"; anything else takes the mask path.
On main since #252.

* Clear an exported object's handle as soon as GraphBLAS frees it

GraphBLAS frees the Matrix or Vector as it hands the arrays over in an
export. _export gave GraphBLAS a copy of the handle and cleared the
object's own handle only at the end, so an error in between, such as
psg's claim_buffer_2d raising for an array UDT, left that handle
pointing at the freed object:

    A.ss.export(format, give_ownership=True) raising after the export,
    then A.nvals
    before: UninitializedObject (a read of freed memory)
    after:  NullPointer, as after an export that succeeds

A plain export, of a copy, also freed that copy a second time when it
was collected, which can abort the process (it did on SuiteSparse 7.4).
GraphBLAS now gets the object's own handle, which it sets to NULL.

* Say an infinity is close only to itself in isclose, as math.isclose does

isclose used math.isclose's formula without its infinity check. With an
infinite x or y, abs(x - y) is inf and so is rel_tol * max(abs(x), abs(y)),
so infinity was close to every number, in binary.isclose (Vector and
Matrix isclose with Numba, Scalar.isclose) and in _isclose_recipe (without
Numba):

    Scalar.from_value(inf).isclose(1.0), .isclose(-inf)
    Vector.from_coo([0], [inf]).isclose(Vector.from_coo([0], [1.0]))
    before: True (all three)
    after:  False, as math.isclose and numpy.isclose say

Two infinities of the same sign are equal, and so still close; NaN is
still close to nothing.

* Compute isclose on integers without wrapping in x - y or abs(x)

isclose computed abs(x - y) and abs(x) in the integer type, where they
wrap: in UINT64, and with Numba in uint64 for any unsigned type (Numba
widens to 64 bits); in INT64 at its extremes, as is abs(-2**63); and
without Numba in every integer type. So, for Vector, Matrix and Scalar:

    UINT64 [0] and [2**64 - 1]:            before True, after False
    UINT8 or UINT32 5 and 6, rel_tol=0.5:  before False, after True
    INT64 -2**63 and 2**63 - 1:            before True, after False
    INT8 -128 and 127, rel_tol=0.5:        before True without Numba,
                                           after False

and Scalar.isclose raised OverflowError for a UINT64 value above INT64's
maximum, because Numba typed the Python int as int64. The difference is
now exact (in uint64, for an integer type), the magnitudes are in FP64,
and Scalar.isclose passes its values in the op's input types, as
ewise_mult does. Mixed types are compared in the type they unify to, as
before, so INT64 with UINT64 is in FP64.

* Keep the imaginary part of x - y in isclose without Numba

_isclose_recipe, which Vector and Matrix isclose use without Numba,
stored x - y in an FP64 Vector before taking abs, and so kept only the
real part of a complex difference:

    Vector.from_coo([0], [0j]).isclose(Vector.from_coo([0], [1j]))
    before: True without Numba (False with it)
    after:  False, as cmath.isclose says

abs is now taken in the complex type first. abs(x) and abs(y) are taken
in the type x - y is in, as the Numba op does, so an FC32 operand beside
an FP64 one is not rounded to FP32 first.

* Count each row's entries when importing coo, so empty rows keep indptr whole

import_coor and import_cooc build CSR (or CSC) index pointers with
indices_to_indptr, which set indptr only at the rows that appear. Every
empty row after the first one left a 0 in indptr, so an empty row in
the middle moved entries to another row, and an empty last row lost
every entry. Exporting as "coo" with sort=True, as "coor" or as "cooc"
and importing back hit it, on main since 2021:

    A = Matrix.from_coo([0, 2], [1, 0], [1.0, 2.0], nrows=3, ncols=2)
    B = Matrix.ss.import_any(**A.ss.export("coo", sort=True))
    before: B holds (2, 1) and (2, 0), and A.isequal(B) said True
    after:  B holds (0, 1) and (2, 0), as A does
    A = Matrix.from_coo([0, 0], [0, 1], [1.0, 2.0], nrows=3, ncols=2)
    before: the round trip gives an empty Matrix
    after:  both entries

It now counts the entries of each row, then sums them. The indices are
converted to uint64 first, so lists work, and one at or beyond nrows (or
ncols) raises IndexOutOfBound, where it was written past the end of
indptr with no bounds check under Numba.

* Read 2-D coo indices as a flat buffer, and count them with numpy without Numba

The new indptr kernel for import_coor and import_cooc indexed a 2-D rows
(or cols) array by its rows, past the end of its first axis:

    rows = np.array([[0, 1], [1, 2]], np.uint64)
    Matrix.ss.import_coor(rows=rows, cols=[0, 0, 1, 1], values=[1, 2, 3, 4],
                          nrows=3, ncols=2)
    before: Segmentation fault with Numba; IndexError without it
    after:  the Matrix from_coo(rows.ravel(), ...) gives, as other
            imports read 2-D indices

Without Numba the kernel ran in Python, once per entry and once per row
(1.1 s for 10**7 rows and 10**5 entries, where the old kernel took 12 ms).
It now counts with np.bincount and sums with np.cumsum (20 ms).

* Compare in the type the dtypes unify to in Scalar.isclose without Numba

Without Numba, Scalar.isclose passed the Python values straight to the
isclose function. Every other path compares in the type the two dtypes
unify to: Vector and Matrix isclose, and Scalar.isclose with Numba,
which passes its values in the op's input types. INT64 and UINT64 unify
to FP64, where 2**62 + 1 rounds to 2**62:

    Scalar.from_value(2**62 + 1).isclose(
        Scalar.from_value(2**62, dtype="UINT64"), rel_tol=0)
    with Numba, and Vector.isclose of the same:  True
    Scalar.isclose without Numba:  False, now True

The values are converted to that type and back to Python numbers, so
x - y still cannot wrap.

* Describe the coo indptr helpers by what they return, not by the old bug

The indices_to_indptr docstring told how the old kernel lost entries,
which the commit that fixed it already says. It now states the
invariant: every empty row gets an empty range. The select test's
docstring no longer names main as a reference point.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants