Skip to content

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

Merged
merged 7 commits into from
Feb 7, 2022
Merged

Ewise-union #159

merged 7 commits into from
Feb 7, 2022

Conversation

eriknw
Copy link
Member

@eriknw eriknw commented Feb 2, 2022

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

def ewise_union(self, other, op, left_default, right_default):

@jim22k what do you think?

@eriknw
Copy link
Member Author

eriknw commented Feb 3, 2022

Pop quiz: what do y'all think x - y will do (or should do) when x and y both have unsigned integer dtypes? Similarly, what about with -x?

@ParticularMiner
Copy link
Contributor

I'm assuming you're hinting at overflows? If so, then perhaps we should follow numpy's style, which is to assume modular arithmetic and in so doing ignore overflows.

@eriknw
Copy link
Member Author

eriknw commented Feb 3, 2022

Yup, that's exactly right. The result stays an unsigned integer. This follows numpy rules.

@jim22k
Copy link
Member

jim22k commented Feb 3, 2022

I'm not 100% sold on the API

def ewise_union(self, other, op, left_default, right_default):

@jim22k what do you think?

I like the ordering, but the names left_default and right_default leave a lot to be desired. Maybe we should think of them as fill values rather than defaults. What do you think of lfill and rfill? I'm all about brevity if we don't lose too much clarity in the names.

@eriknw
Copy link
Member Author

eriknw commented Feb 3, 2022

I don't like rfill and lfill.

How about default, which may be a single scalar or an iterable with two items? I suspect it may be common to use the same value for both the left and the right.

btw, SuiteSparse:GraphBLAS calls these alpha and beta.

@eriknw
Copy link
Member Author

eriknw commented Feb 4, 2022

An even funkier spelling that is both concise and convenient could be:

def ewise_union(self, other, op, *defaults):

where defaults could be length 1 or 2 (or 0 if we ever want a default value).

@eriknw
Copy link
Member Author

eriknw commented Feb 4, 2022

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.

@eriknw
Copy link
Member Author

eriknw commented Feb 7, 2022

Merging as is. @jim22k we can continue discussing the function signature and make changes before releasing if necessary.

@eriknw eriknw merged commit 28700cf into python-graphblas:main Feb 7, 2022
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.

ENH: use GxB_eWiseUnion when using x - y infix IDEA: allow strings to be used in place of operators A.mxm(B, 'min.+')
3 participants