Repository navigation
Make apply accept IndexUnaryOps - #229
Conversation
Add IndexUnaryOp and typed versions New gb.indexunary namespace
|
This is the minimum effort to make this work. A future PR will address user-defined versions for both IndexUnaryOp and SelectOp. However, @eriknw if you feel strongly that we need to fix the way things work for Vectors (i.e. must use rowindex, rowle, etc) as part of this PR, I am okay adding that. Or we can push it to a future PR as well. |
eriknw
left a comment
There was a problem hiding this comment.
w00t! Thanks Jim! I had mostly (entirely?) minor nits. You know, it's already been super-nice having select operators. I forget where IndexUnary operators will be used. Less often, though, right? It was a good call to separate them.
I'm fine taking an incremental approach to PRs. One should implement what one is comfortable with, and hopefully anything still to be done is noted somewhere or intended to be implemented soon-ish.
Btw, a speaker at PyCon gave an impassioned speech about about reaching and staying at 100% coverage as a virtue and making your project better. So, this is a friendly reminder to please look at all missed coverage at https://coveralls.io/github/python-graphblas/python-graphblas. We're not sticklers about coverage (well, I kinda am for myself), but good coverage is appreciated. I think I may re-enable the coveralls chat bot. I suppose it wasn't that noisy, I just wasn't used to it is all.
| semiring_from_string, | ||
| # Give binary precedence over this | ||
| select_from_string, | ||
| indexunary_from_string, |
There was a problem hiding this comment.
What would one expect to get from e.g. op.from_string("tril")? Should we look for select before indexunary, b/c select is more common?
And, remind me, when we register a new UDF, do we plan to create both versions, or just the opclass that is being registered?
There was a problem hiding this comment.
Yes, we should look for select before indexunary. If it exists in select, that is most likely what the user wanted. And if you do op.from_string("rowindex"), you will get the indexunary version because it doesn't exist in select. So anything returning bool will default to select while everything else will be apply.
For UDFs, I haven't decided yet what to do. My initial thought is to register all places where it makes sense. So always register the indexunary, and additionally register with select if it returns a bool.
|
And sorry about the failed tests. I was stupid. Merge with main for a fix. |
|
As far as coverage goes, I know I'm missing several lines of code which get tested once I add UDFs. Until then, I don't want to remove that code or I might forget to add it back in. |
applyis treated identically when calling into SuiteSparse.