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 — Extend the monoid guard to built-ins that carry a UDT fallback func by eriknw · Pull Request #619 · python-graphblas/python-graphblas Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #619 +/- ## ========================================== + Coverage 98.74% 98.86% +0.12% ========================================== Files 105 105 Lines 31099 31149 +50 Branches 3647 3652 +5 ========================================== + Hits 30709 30796 +87 + Misses 249 220 -29 + Partials 141 133 -8 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The guard added with the monoid-from-built-in rejection keyed on "constructed with neither a Python func nor a numba func", which is true for most built-ins but not all of them: binary.any, first, pair, and second carry a Python orig_func as their UDT fallback while their typed ops are still TypedBuiltinBinaryOp. All four sailed past the guard and reproduced exactly what it exists to prevent: an opaque AttributeError partway through registration, with binary.<name>._monoid left pointing at the half-built monoid (binary.any.monoid is monoid.any flipped from True to False after the rejected call). binary.any is even named in the guard's own rationale as an op whose identity SuiteSparse ignores. Key the probe on the typed ops as well: any TypedBuiltinBinaryOp among them marks the op built-in. The two signals agree everywhere else, including under mapnumpy=True, where the mapped numpy binary ops hold the built-in's typed ops and no funcs, and are flagged by both tests alike; the numpy monoids themselves are mapped without passing through Monoid._build, so their registration is unaffected (verified under both mapnumpy settings). A UDF's typed ops are never TypedBuiltinBinaryOp, so nothing legitimate is newly rejected.
…/max
Behavior fix. monoid.min and monoid.max generate a UDT's identity field
by field, and _scalar_identity picked the integer arm with
np.issubdtype(dtype, np.integer). numpy files timedelta64 under
np.integer, so a record with a timedelta64 field reached np.iinfo, which
rejects it:
rec = dtypes.register_anonymous(np.dtype([("n", np.int64), ("t", "m8[s]")]))
monoid.min[rec] # before: ValueError: Invalid integer data type 'm'.
rec in agg.min # before: the same ValueError, not False
A datetime64 field already got "KeyError: monoid.min does not work with
...: field 't' has unsupported dtype datetime64[s]", and `in` answered
False. Test the dtype's kind instead, so a timedelta64 field takes that
path too:
monoid.min[rec] # after: KeyError ... field 't' has unsupported dtype timedelta64[s]
rec in agg.min # after: False
Real identities for timedelta64 fields (its own max and min, with NaT
in mind) are a separate enhancement.
Behavior fix. Monoid._compile_udt handed a binary op whose return type
differs from its input type straight to GrB_Monoid_new_UDT, which
rejects it with a DomainMismatch that has no message, also from `in`:
op = BinaryOp.register_anonymous(lambda x, y: x["a"] + y["a"], is_udt=True)
m = Monoid.register_anonymous(op, 0)
m[rec] # before: DomainMismatch (empty message)
rec in m # before: the same DomainMismatch
Check the compiled op's return type first and raise a TypeError, the
error the neighbouring check (two different input types) raises:
m[rec] # after: TypeError: monoid.<lambda> does not work with rec: a
# monoid's binary operator must return its input type, rec, not INT64
rec in m # after: False
Behavior fix. The lazy branch stored register_new's arguments for the
delayed call but left is_idempotent out, so the monoid built on first
access was never idempotent:
Monoid.register_new("m", op, 0, is_idempotent=True, lazy=True)
monoid.m.is_idempotent # before: False
# after: True
The eager branch already passed it.
The test registers lazy_idem in the binary and monoid namespaces, so
test_operator_types ignores it like the other names test_lazy_op makes.
Behavior fix. Monoid._compile_udt generated a field-wise identity
whenever the monoid had no identity and its name was plus, times, min or
max. A user monoid gets its name from its BinaryOp, which gets it from
the function, so a user op called `min` (or a monoid given name="plus")
silently took the built-in's identity and gave wrong results. With an op
that takes the field-wise max and identity None:
op = BinaryOp.register_anonymous(fieldwise_max, "min", is_udt=True)
v = [(1, 2), (5, -3)]; w = [(-1, -2), (-5, -3)]
Monoid.register_anonymous(op, None) # name "min"
before: identity (INT64_MAX, INT64_MAX); v.reduce -> (INT64_MAX, INT64_MAX)
Monoid.register_anonymous(op, None, "plus")
before: identity (0, 0); v.reduce -> (5, 2), w.reduce -> (0, 0)
both, after: KeyError: monoid.<name> does not work with ...: no identity value
Generate the identity only for a built-in monoid, the ones with no
_binaryop of their own. A user monoid with an identity is unaffected.
test_udt_monoid_op_must_return_its_input_type matched the error against "_BuiltinOpsRec", the name the record_udt fixture registers. But register_anonymous returns the one DataType for an np.dtype and gives it the newest name, and test_udt_op_pickle registers the same record dtype as "_PickleUdt". In random order, with a record_udt test, then test_udt_op_pickle, then this test, the message said "_PickleUdt" and the match failed (wave 2 CI: 13 jobs, e.g. 122634118659 and 122634118406, on both backends and with mapnumpy on or off). Match the fixture's name as it is when the error is raised.
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.