GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fix UDT, dtype and JIT edge cases found in #591's review by eriknw · Pull Request #663 · python-graphblas/python-graphblas Conversation
|
| # | finding | now | recommendation |
|---|---|---|---|
| 1 | "Erik" row: Scalar.__iadd__ vs Vector.__iadd__ |
INT8 -1 += 0.5: Scalar -1 (accumulates, 0.5 cast to INT8 first), Vector 0 (v << v + 0.5, computed in FP64) |
Make Scalar's += s << s + x, as Vector's and Matrix's are, so infix means one thing for all three. Low priority. |
| 2 | s1.7 dict literal keys | extra keys ignored (== {"a": 1, "b": 2, "zz": 3} compares), a missing key is a bare KeyError: 'b' |
Refuse both with one ValueError naming the missing and unexpected keys, as a tuple of the wrong length is refused. The place is udt._as_record_parts now. |
| 3 | s2.3 first/second/any result types | keep one operand's type; plus promotes per leaf; built-in INT8 first INT32 is INT32 | Keep the behavior (values are untouched, and storing into another UDT now fuses, s2.4); change udt.rst's "since they don't touch field values" to say they keep the operand's type. |
| 4 | s2.6 out-of-order offsets | first on a record with offsets out of field order gives a reordered anonymous UDT |
Leave it (rare layout); if wanted, have _resolve_udt_return_type return an input type whose fields match by name. |
| 5 | new: eq/ne on records in an array | with Numba, Numba's raw message in UdfParseError (eq(Record(...), Record(...))); without Numba, Scalar == compares (numpy compares records) |
Refuse with the s2.5 KeyError for now; comparing field by field in the eq wrapper is the full fix. I left it because the two paths disagree. |
| 6 | new: Scalar.from_value(300, INT8) |
wraps to 44 for a GrB_Scalar but raises OverflowError for a C scalar; so ibo(300, dtype=INT8) binds 44 |
Pick one: wrap in both, as int8_vec[0] = 300 stores, or raise in both, as numpy 2's np.array(300, np.int8) and int8_vec + 300 do. I first leaned to wrapping (the store rule); rB leans to raising, since from_value builds thunks and thetas, where a silent 44 changes what the op means. I now agree with rB: raise in both. |
| 9 | new (rB 11): any could fuse into another UDT |
b << a.ewise_mult(c, binary.any) raises DomainMismatch |
any on two UDT types already returns the second's type and value, so a fused any that casts y is right. Leave it refused for now; fuse it as second if anyone asks. |
| 7 | new: IndexUnaryOp thunk beyond int64 | v.apply(indexunary.rowle, 2**70) and Scalar.from_value(2**63) still give cffi's bare "int too big to convert" (s3.6 fixed only IndexBinaryOp's theta) |
Give Scalar.from_value itself the message for an inferred int beyond INT64, and consider typing 263..264-1 as UINT64, as np.asarray does. That changes from_value's typing, so it's a question. |
| 8 | new: lifted ops without Numba | rec + rec, -rec raise KeyError: plus does not work with R with no hint (only eq/ne got it) |
Add the same hint in OpBase.__getitem__'s _udt_types is None branch for a lifted name. Low. |
| 10 | new (bxp): psg's claim_buffer_2d with a subarray dtype raises on spare values and scrambles Fortran order |
python-graphblas no longer calls it for array UDTs | Report it upstream (your call). The MRE is in the psg-ai workspace, mres/claim_buffer_2d_subarray/, with a letter in its inbox. |
| 11 | new (rB): a monoid over an is_udt op, given an identity of another kind: Monoid.register_anonymous(op_udt, np.float64(0.0))["INT64"] |
raises TypeError: an integer is required (from Scalar.from_value on a C scalar); an ordinary op's monoid casts the identity with ffi.cast, so 0.5 becomes 0 for INT64 without a word (pre-existing) |
Make both agree: take an identity that converts exactly (0.0 to INT64), and refuse one that does not (0.5), in both. Low. |
| 12 | new (rB): to_dense/to_coo(dtype=) without a fill casts with numpy, while to_dense(fill_value, dtype=) stores |
beyond array to array (fixed here) they disagree: between record UDTs numpy casts by position whatever the field names, and between a built-in and an array type it gives INT64[4] to FP64 as (n, 4) and FP64 to FP64[4] with each number repeated, while the store path raises DomainMismatch | Apply the store rule (_udt_cast_error) in normalize_values too, so the two paths agree; a follow-up. |
| 13 | new (rB): isclose treats a finite complex whose magnitude overflows (beyond about 1.8e308) as infinite | cmath.isclose checks the parts instead and would call it close to a near value |
Leave it. It needs numbers near the float limit in both parts. |
JIT
| id | question | recommendation |
|---|---|---|
| s2.1 (3) | Should a call that raises JitError be retried once? SuiteSparse sets "load" when a compile fails, so the retry would run the cfunc, with a NoJITWarning naming the failed compile. That would make "the op still works" true for anything the names and macros miss, such as a platform's own macro as a field. | Not now. After (1) and (2), only unusual field names reach it, and a retry in base.call changes every call's error path, and assumes a failed JIT leaves the output untouched, which I have not verified for every SuiteSparse method. Revisit if a report comes in. |
| s2.7d | Non-ASCII UDT and field names go to the C compiler, and type names reach kernel file names (libGB_jit__add__...__plus_gbudt_Äpfel.dylib). Not reproduced: clang 19 and gcc 15 accept UTF-8 identifiers. GCC < 10 rejects them ("stray '\303'"; seen here only as gcc-15 -std=c89). MSVC was not tried. |
Give a non-ASCII type name a synthetic C name (free: it keeps the JIT). Leave field names until someone reports a failure. |
| F4 (r652f) | A failed lazy probe, at the first UDT op with a C compiler that does not work, leaves SuiteSparse's "load" for the rest of the process. A failed probe at init sets "run", for the reason it gives: "load" still loads cache kernels that other builds of this version may have written. | Set "run" after a failed lazy probe too, in a follow-up. It is not obvious enough to do here: _compiling_c_source detects a failed probe by the "load" it leaves, so both change together. |
| first imports | graphblas.core.dtypes, core.ss.matrix, core.ss.prefix_scan and core.ss.vector also fail as a process's first import (main too). |
Low value (private modules); leave, or fix with the same reorder pattern when those modules are next touched. |
🤖 Generated with Claude Code
Sorry, something went wrong.
There was an error while loading. Please reload this page.
1287305 to
9a75b1f
Compare
9a75b1f to
a74e786
Compare
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@ ## 54-jit-on-windows-cmake #663 +/- ## =========================================================== - Coverage 99.30% 99.19% -0.11% =========================================================== Files 109 109 Lines 35211 35823 +612 Branches 4010 4096 +86 =========================================================== + Hits 34967 35536 +569 - Misses 151 171 +20 - Partials 93 116 +23 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
648d71d to
d047afe
Compare
a03193b to
adc6514
Compare
Its docstring said None was "only" for an unsupported field type or a field name that is not a C identifier. It is also None for a packed record (numpy's default without align=True), a record with an array field, and a UDT that is neither a record nor an array (bytes, a date).
"Select a UDT's elements equal or unequal to a value, as == compares
them" built the binary.eq (or ne) apply only when the select expression
ran, so its errors moved from the call to .new(), and a thunk of another
type lost the select's own message:
v.select("==", Scalar of a record UDT with other field names)
before: an expression; .new() raised eq's KeyError
after: KeyError at the call (record UDTs must share field names)
v.select("==", (1, 2.0)) without Numba: the same, eq's KeyError at the call
v.select("==", some_vector), v.select("==", object())
before: TypeError: Vector can't be directly converted to a numpy array /
int() argument must be ... not 'object'
after: TypeError: Bad type for keyword argument `thunk=` in Vector.select
Vector.select and Matrix.select now convert the thunk in the one block
every select uses, typed by eq for a UDT comparison, and build the apply
(typed, not computed) at the call; _select_by_comparison computes it when
the expression runs. From rB's patch.
"Compare an int beyond 64 bits in Scalar.isequal as == compares it" left
isclose with the same path:
Scalar.from_value(1, INT8).isclose(2**63)
before: OverflowError: int too big to convert
after: False
Scalar.from_value(2.0**64).isclose(2**64): before OverflowError, after True
Such an int is compared in Python by isclose's formula; one too large for
a float, beside a float, is not close.
"Compare a str that bytes cannot hold as unequal to a bytes UDT" put its
branch under the check that the literal is no numpy value, so an np.str_
(a str subclass, and a numpy scalar) missed it:
s4_vec == np.str_("\xe9")
before: KeyError: binary.eq does not work with (S4, dtype('<U1')): a UDT
that is neither a record nor an array pairs only with its own type.
after: all False, as numpy and s4_vec == "\xe9" give
The bytes check now runs first, for any str.
"Refuse an array literal that does not broadcast with an array UDT by
name" typed such an array in its own shape, which for an empty array is
a layout no UDT can have:
i8x3_vec + np.array([]) (also ==, np.zeros((2, 0)), np.zeros((0, 3)))
before: ValueError: Unknown dtype: ('<f8', (0,)) ...
after: ValueError: could not broadcast input array from shape (0,)
into shape (3,)
as before that commit for (0,), and now for the empty arrays that
broadcast too.
"Refuse a tuple of types an op does not take with a TypeError" said
"binary.plus[...] takes one dtype or two, not 1" for binary.plus[("INT64",)],
which reads as a contradiction. It now says "takes a dtype or a pair of
dtypes, not a tuple of 1" (and of 3), and an IndexBinaryOp "takes a dtype,
or a tuple of two (x and y) or three (x, y and theta), not a tuple of 4".
"They need Numba" covered the whole list above it, but pair, count, agg.ss.first and agg.ss.last work on a record UDT without Numba (first, second, any, identity and the lifted ops raise KeyError).
import graphblas.core.ss.binary as the first graphblas import raised
ImportError ("cannot import name 'register_new' from partially initialized
module"), and so did graphblas.core.ss.unary, on main too. The module
imported the operator package at its top; that initializes the operators,
which imports graphblas.binary.ss, which takes register_new from the
module before it was defined.
Now register_new comes first and the operator imports and the
TypedJitBinaryOp (or TypedJitUnaryOp) class follow it, so register_new is
there when the operators look for it. Nothing else changes; the function
uses those names only when called.
…ses cmake The module's setup called fix_jit_config(), which sets "on" whenever it finds a compiler to repair from. Where SuiteSparse compiles through cmake (always when it is built with MSVC), that would compile every kernel of the module through cmake, about 10 s each on Windows CI, if a runner has conda's clang.exe or the build machine's cl.exe. UDT work already leaves the default "run" alone there (_enable_jit_for_udt); the setup now does the same, so only an "on" set before the tests compiles there. The setup moves into _configure_jit_for_tests, which a test drives with a stand-in config on every host.
GraphBLAS stores an array UDT's element as one contiguous block, in row
or column order of the entries. In "fullc" and "bitmapc", export built
the values with psg's claim_buffer_2d in Fortran order, which made the
element's own axes the slowest, and import made the whole array Fortran
order before handing it over. Both scrambled the numbers, on every
SuiteSparse version, and an unpack then pack round trip worked only
because the two scrambles cancelled:
exp = np.arange(24).reshape(2, 3, 4); A = Matrix.from_dense(exp)
A.ss.export("fullc")["values"][0, 1] before: [2, 4, 18, 20]
Matrix.ss.import_fullc(values=exp)[0, 1] before: [5, 17, 9, 21]
after: both [4, 5, 6, 7], as A[0, 1]
Column-order export now reshapes the claimed buffer to
(ncols, nrows, *element) and swaps the first two axes; row order stays
with claim_buffer_2d, which gets it right and whose array owns the
buffer, so unpack then pack(take_ownership=True) stays zero-copy (a test
checks it). Import swaps the axes into a C-contiguous copy, so a
column-major array UDT given one element per entry is never taken
without a copy. Numbers given flat, in column order, import as before.
The values are converted once, to see their shape: straight to Fortran
order unless they are an array-UDT ndarray, and not copied again when
that made a new array.
export(raw=True) gives an array UDT's values as a list of elements, of
shape (n, *element), and in "fullc" and "bitmapc" that list is in
column order already. Import made it Fortran order, as for a 2-d array
of entries, so the numbers went in scrambled and the raw round trip
failed, on every SuiteSparse version:
A = Matrix.from_dense(np.arange(24).reshape(2, 3, 4))
B = Matrix.ss.import_any(**A.ss.export("fullc", raw=True))
before: B[0, 1] is [5, 17, 9, 21], and B.isequal(A) is False
after: B[0, 1] is [4, 5, 6, 7], and B.isequal(A) is True
Such a list, numbers flat in column order, and one element (an iso
value; one of INT64[2, 2] was read transposed) are now kept in C order,
so take_ownership=True takes them without a copy. The docstrings of
import_fullc and import_bitmapc say so. Values whose trailing axes fit
none of these now raise ValueError, where they were scrambled the same
way:
Matrix.ss.import_fullc(values=np.arange(24).reshape(2, 3, 4), dtype="INT64[2, 2]")
before: [0, 1] is [[5, 17], [9, 21]] (import_fullr gives [[4, 5], [6, 7]])
after: ValueError: values of INT64[2, 2] in column order must have
shape (nrows, ncols, 2, 2), an element per entry, ...
GraphBLAS may hand over a values buffer with more elements than
nrows * ncols: SuiteSparse 7's memory pool rounds every allocation up to
a power of two, and a full matrix resized smaller keeps its buffer. For
an array UDT, psg's claim_buffer_2d then reshaped the values to
(nrows, ncols), which raised ValueError. GraphBLAS had already given the
buffer away, so unpack left the Matrix empty:
SuiteSparse 7.4: Matrix.from_dense(np.arange(24).reshape(2, 3, 4)).to_dense()
SuiteSparse 10.5.2:
A = Matrix.from_dense(np.arange(36).reshape(3, 3, 4)); A.resize(2, 3)
A.ss.unpack("fullr")
before: ValueError: cannot reshape array of size 24 into shape (2,3)
(on main; the second also leaves A with nvals 0)
after: the values, of shape (2, 3, 4)
The spare elements are now cut off before the reshape, as psg does for
other types. On SuiteSparse 7 every dense export of an array UDT failed,
test_subarray_dtypes among them. Row order with an exactly sized buffer
still goes to claim_buffer_2d, whose array owns the buffer, so unpack
then pack(take_ownership=True) stays zero-copy.
Its loop over the to_* and from_* methods rebuilt B and asserted nothing, and its from_dense(to_dense()) check compared Full2 with itself. Both now compare with the matrix they came from. They pass before and after; the test only checks more.
…nstructor
A Monoid over an is_udt=True BinaryOp compiles per dtype in
_compile_udt, which always called GrB_Monoid_new_UDT. For a built-in
type that the op returns, SuiteSparse refuses that constructor, since
the op's output is not a user-defined type:
op = BinaryOp.register_anonymous(lambda x, y: x + y, is_udt=True)
Monoid.register_anonymous(op, np.float64(0.0))[FP64]
before: DomainMismatch with an empty message
after: the monoid, FP64 -> FP64, and v.reduce(it) sums
A built-in return type now goes through GrB_Monoid_new_<type>, as a
monoid over an ordinary BinaryOp does in _build.
to_dense, to_coo, to_csr and the like convert the exported values with
values.astype(dtype.np_type). The values already hold each element's
own axes last, and numpy's astype to an array dtype adds that dtype's
axes again, repeating each number into a whole element:
v = Vector.from_coo([0, 1], np.arange(8).reshape(2, 4)) # INT64[4]
v.to_dense(dtype="FP64[4]").shape (an iso v too, and Matrix)
before: (2, 4, 4)
after: (2, 4), the same numbers as FP64
Element shapes follow the store rule that to_dense(fill_value, dtype)
already uses: they must match apart from leading axes of length 1, so
FP32[1, 4] gives (2, 1, 4) and FP64[2] raises DomainMismatch, where it
gave (2, 4, 2).
A serialized object keeps its type's C name. For a type registered with
dtypes.ss.register_new, that is the name it is registered under in
dtypes.ss, where lookup_dtype does not look. The fallback, the numpy
repr serialize writes as GrB_NAME, is empty for such a type, and
literal_eval("") raised:
T = dtypes.ss.register_new("pair", "typedef struct { int64_t a ; double b ; } pair ;")
Matrix.ss.deserialize(Matrix(T, 2, 2).ss.serialize())
before: SyntaxError: invalid syntax
after: a Matrix of T (SuiteSparse checks the blob against it, as
for dtype=T)
dtypes.ss is tried last, after GrB_NAME, so a numpy UDT's own repr
still wins. When nothing resolves, deserialize now raises a ValueError
that says to pass the type as dtype=: a blob of such a type that this
process has not registered, or of an anonymous UDT whose numpy repr was
too long to keep (two tests expected that bare SyntaxError).
On Numba 0.58 and 0.59, a UDT UDF reading a field its record lacks
raised NumbaPendingDeprecationWarning under -W error, as in our tests:
the warning filter in _numba_compile never matched, because the
warning's text need not start with the words it looked for. Without
-W error, users got the first line of Numba's old_style TypingError:
b = BinaryOp.register_anonymous(lambda x, y: x.zzz, is_udt=True)
b[R] before: UdfParseError: binary.b does not work with (R, R):
Failed in nopython mode pipeline (step: nopython frontend)
after: UdfParseError: binary.b does not work with (R, R):
Field 'zzz' was not found in the record
as Numba 0.60 and later already gave. x["zzz"] likewise now gives
Numba's "Field 'zzz' was not found in record with fields (...)" line,
which old_style quotes, instead of that first line.
…the type
Before SuiteSparse 9 a blob has no GrB_NAME to fall back to. There,
deserializing a type from C source that this process has not
registered, or a UDT whose C name does not resolve, said only:
Matrix deserialize cannot resolve dtype 'pair' and
GxB_Serialized_get_String is unavailable in this SuiteSparse build
which test_jit_udt_deserializes_without_dtype did not expect, so it
failed on SuiteSparse 8. Every build now gives the message that later
ones give, with the remedy:
Matrix deserialize cannot tell the dtype from the type name 'pair';
pass it as dtype=
Numba 0.60 raises a bare KeyError for x["zzz"] on a record without that
field, as for x.zzz, but its argument is Numba's sentence rather than
the name, and the KeyError branch wrapped it once more:
before: Field "Field 'zzz' was not found in record with fields
('a', 'b')" was not found in the record
after: Field 'zzz' was not found in record with fields ('a', 'b')
as other Numba versions say. Only an argument that is an identifier, as
x.name needs, is taken for the field's name.
Scalar.isclose compares an int that INT64 cannot hold in Python, with the
formula written out, which lacked isclose's infinity check:
Scalar.from_value(inf).isclose(2**70)
before: True
after: False, as math.isclose says
It now calls the function that binary.isclose compiles, as the other
paths do since "Say an infinity is close only to itself in isclose, as
math.isclose does".
"Select a UDT's elements equal or unequal to a value, as == compares them" sends UDT eq and ne through select's rewrite of A.select(A == x). Before "Select by a transposed comparison as by its mask, not by the untransposed one", that rewrite also matched A.select(A.T == x) and dropped the transpose. test_udt_select_by_equality now uses a 2 x 3 matrix with entries off the diagonal, so that a lost transpose would show, checks A.T.select(A.T == x) and the DimensionMismatch of A.select(A.T == x), and assigns a select through a mask with replace.
An iso Matrix exports as "coo" with its indices and one value. The
indices came from GrB_Matrix_extractTuples_<type name>, which for a UDT
does not exist, and the value from a reduce with monoid.any, which a UDT
can do only with Numba:
A = Matrix.ss.import_fullr(values=[[1, 2, 3, 4]], nrows=2, ncols=3, dtype="INT64[4]", is_iso=True)
A.ss.export("coo")
before: AttributeError: ... no function ... named 'GrB_Matrix_extractTuples_<name>'
after: values [[1, 2, 3, 4]], of shape (1, 4), and import_any gives A back
A UDT now uses GrB_Matrix_extractTuples_UDT, as to_coo does, and the
value is read with iso_value's iterator into an array of one element,
so an array UDT's value is (1, *element), as other iso exports give it
(np.array([value], dtype) repeated each number into an element). An iso
Matrix with no entries still exports 0, as the reduce gave.
adc6514 to
386a561
Compare
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.