Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add select Operation #207

Merged
merged 9 commits into from Apr 14, 2022
Merged

Add select Operation #207

merged 9 commits into from Apr 14, 2022

Conversation

jim22k
Copy link
Member

@jim22k jim22k commented Apr 11, 2022

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.
@jim22k jim22k requested a review from eriknw April 11, 2022 21:28
@jim22k jim22k changed the title Select Add Select Operation Apr 11, 2022
@jim22k jim22k changed the title Add Select Operation Add select Operation Apr 11, 2022
@eriknw
Copy link
Member

eriknw commented Apr 11, 2022

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.

grblas/_automethods.py Outdated Show resolved Hide resolved
grblas/matrix.py Outdated Show resolved Hide resolved
grblas/operator.py Outdated Show resolved Hide resolved
grblas/operator.py Outdated Show resolved Hide resolved
grblas/operator.py Outdated Show resolved Hide resolved
grblas/select/__init__.py Outdated Show resolved Hide resolved
grblas/operator.py Outdated Show resolved Hide resolved
@eriknw
Copy link
Member

eriknw commented Apr 12, 2022

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.

Copy link
Member Author

@jim22k jim22k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

grblas/operator.py Outdated Show resolved Hide resolved
grblas/operator.py Outdated Show resolved Hide resolved
grblas/operator.py Outdated Show resolved Hide resolved
grblas/select/__init__.py Outdated Show resolved Hide resolved
- Use correct dtype based on thunk
- Handle Scalar thunk properly
- Rename IndexUnaryOp to SelectOp
graphblas/vector.py Outdated Show resolved Hide resolved
@eriknw
Copy link
Member

eriknw commented Apr 13, 2022

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

@jim22k
Copy link
Member Author

jim22k commented Apr 13, 2022

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

Copy link
Member

@eriknw eriknw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

graphblas/matrix.py Show resolved Hide resolved
graphblas/operator.py Show resolved Hide resolved
graphblas/operator.py Show resolved Hide resolved
graphblas/select/__init__.py Outdated Show resolved Hide resolved
graphblas/select/__init__.py Show resolved Hide resolved
graphblas/select/__init__.py Outdated Show resolved Hide resolved
@eriknw
Copy link
Member

eriknw commented Apr 14, 2022

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
Copy link
Member

eriknw commented Apr 14, 2022

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

@eriknw eriknw merged commit a0051c4 into python-graphblas:main Apr 14, 2022
@eriknw
Copy link
Member

eriknw commented Apr 14, 2022

This is in, thanks so much @jim22k 🎉

@jim22k jim22k deleted the select branch April 19, 2023 20:04
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.

None yet

2 participants