Add `select` Operation by jim22k · Pull Request #207 · python-graphblas/python-graphblas

jim22k · 2022-04-11T21:27:57Z

This implements item 1 in the proposal for adding select.

Put the IndexUnaryOps which return bool into the grblas.select namespace
Allow these to be called directly (gb.select.tril(A)) to perform a select operation
Include the "special" versions (select.row, select.column, select.value), but also include the "raw" versions (select.rowle, etc)
Add a select method (e.g. A.select("tril"), A.select("<", 1), A.select(gb.select.diag, 1), A.select("row<=", 1))

Two ways of calling: 1. A.select(op, thunk) 2. gb.select.op(A, thunk) When calling `A.select(op)`, the op may be passed as a string. An additional set of "special" select functions take a comparator expression. An example: `gb.select.row(A <= 2)`. Row, column, and value are included, indicating what part of A is being compared in the select statement.

eriknw · 2022-04-11T22:12:53Z

Aha, I see you found errors in the bizarro tests: https://github.com/metagraph-dev/grblas/blob/8e9acb9c1ac0e5689fc2f1066fd60020ea1c3ffe/.github/workflows/test_and_build.yml#L102-L109

This trick is kinda weird, but it's the best way I know to test both types of scalars (C and GrB) throughout the API. You may find it handy to switch to bizarro world locally with this command:

#!/bin/bash

find grblas -type f -name "*.py" -print0 | xargs -0 sed -i -s \
    -e '/# pragma: is_grbscalar/! s/is_cscalar=False/is_cscalar=True/g' \
    -e '/# pragma: is_grbscalar/! s/is_cscalar = False/is_cscalar = True/g' \
    -e '/# pragma: to_grb/ s/is_cscalar=True/is_cscalar=False/g' \
    -e '/# pragma: to_grb/ s/is_cscalar = True/is_cscalar = False/g'

Scalars are GrB by default, but there are a few places in the code where they must be a specific type of scalar.

When creating a scalar with is_cscalar=True, it is assumed that it must be a C scalar.
- Add # pragma: to_grb if it is allowed to be GrB
When creating a scalar with is_cscalar=False, it is assumed it is also safe to change it to a C scalar.
- Add # pragma: is_grbscalar if it must be a GrB scalar

Once you apply the above search-and-replace, you can't search-and-replace back the the original. Sometimes using patches is handy if you need to make changes.

eriknw · 2022-04-12T02:58:55Z

We added my_op.is_positional attribute to operators for ones like positioni and firsti. I think IndexUnaryOps should also have .is_positional attribute, and I think it should be true for all IndexUnaryOps except those with "VALUE" in the name. Then, in select, the thunk should be treated as int64 if the op is positional unless the int32 version of the operator was explicitly given.

jim22k

I'm going to change IndexUnaryOp to SelectOp. When we add apply with index unary, I will add that in as a different op class.

- Use correct dtype based on thunk - Handle Scalar thunk properly - Rename IndexUnaryOp to SelectOp

eriknw · 2022-04-13T14:56:07Z

Please try to increase coverage: https://coveralls.io/builds/48230528

jim22k · 2022-04-13T20:03:28Z

@eriknw I think this is ready. Please take one last look, and if you're happy with it, go ahead and merge.

eriknw

It occurs to me that several of the SelectOps are awkward for Vectors. gb.select.row needs to be used instead of gb.select.column, but column-wise selectors are technically valid on Vectors, but they don't do much. Similarly, tril is kind of useless. diag and offdiag can be use to select a specific index or all except a specific index, but the thunk to indicate which index needs to be negative.

Should we consider having type-specific (Vector, Matrix) SelectOps? There's definitely some awkwardness around UnaryOps/SelectOps, isn't there?

This is super-close to merging. I'll nag you about coverage on the next select PR ;)

eriknw · 2022-04-14T04:05:55Z

Also, I tried select on a Matrix with a UDT, and it worked! Can you add a simple example to test_udt in "test_matrix.py"? For example, you could verify that these give the same result: gb.unary.one(gb.select.tril(M)) and gb.select.tril(gb.unary.one(M)).

eriknw · 2022-04-14T05:06:44Z

For warm-fuzzies, you may want to merge with main branch and update to python-suitesparse-graphblas 7.0.3.1.

eriknw · 2022-04-14T20:44:57Z

This is in, thanks so much @jim22k 🎉

jim22k added 3 commits April 11, 2022 15:14

Merge branch 'main' into select

0955311

Fix import sorting

9f4f891

jim22k requested a review from eriknw April 11, 2022 21:28

jim22k changed the title ~~Select~~ Add Select Operation Apr 11, 2022

jim22k changed the title ~~Add Select Operation~~ Add select Operation Apr 11, 2022