-
Notifications
You must be signed in to change notification settings - Fork 15
Ewise-union #159
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
Ewise-union #159
Conversation
Pop quiz: what do y'all think |
I'm assuming you're hinting at overflows? If so, then perhaps we should follow |
Yup, that's exactly right. The result stays an unsigned integer. This follows numpy rules. |
I like the ordering, but the names |
I don't like How about btw, SuiteSparse:GraphBLAS calls these |
An even funkier spelling that is both concise and convenient could be: def ewise_union(self, other, op, *defaults): where |
Okay, I think this (and all my current PRs) is ready pending a final decision on function signature. Current options: 1. def ewise_union(self, other, op, left_default, right_default):
2. def ewise_union(self, other, op, left_default, right_default=None):
3. def ewise_union(self, other, op, lfill, rfill):
4. def ewise_union(self, other, op, left, right):
5. def ewise_union(self, other, op, alpha, beta):
6. def ewise_union(self, other, op, default):
7. def ewise_union(self, other, op, *defaults): My top two choices are 1 and 2. |
Merging as is. @jim22k we can continue discussing the function signature and make changes before releasing if necessary. |
Fixes #154
Fixes #148
This also added a
_GrbScalar
wrapper for making C calls.Docstrings and tests still needed.
For now, the implementation for
ewise_union
is SuiteSparse-specific. We could make a recipe when needed.Also, hooray for #157, which made adding
ewise_union
method that much easier!I'm not 100% sold on the API
@jim22k what do you think?