Graphegon / pygraphblas

GraphBLAS for Python
https://graphegon.github.io/pygraphblas/pygraphblas/index.html
Apache License 2.0
343 stars 27 forks source link

sub, rsub and isub #70

Closed michelp closed 4 years ago

michelp commented 4 years ago

I'd propose a behavior change of the __sub__ operator to align with how the other operators work. Right now v - w will do the equivalent of v + (-w) where the neg takes the inverse of w. I think it should align instead with what __add__ does and use v.eadd(w, add_op=MINUS). The old behavior can still be had by adding the negation. This makes all the binary operations consistent, and allows me to make a scalar construction like w - 5 which would become w.apply_second(MINUS, 5). Any thoughts @szarnyasg and @marci543 ? I made a PR #69.

szarnyasg commented 4 years ago

@michelp Good question... this can get a bit counter-intuitive (which is a challenge when trying to turn it into a Pythonic API). I had a slide about this in my tutorial:

image

There are at least two questions:

michelp commented 4 years ago

I agree it has some counter intuitive behavior, but consistency with GraphBLAS I think is key here. And the old behavior can be had with v + (-w). As for the two questions:

1: The zero value is kept, this lines up with GraphBLAS where it's really only monoids and semirings that have an "implicit zero" value when they are applied. Unstored values in matrices and vectors are uninterpreted until a monoidal operation applies.

2: Same things GraphBLAS does, +x. Since there aren't two values the operator doesn't apply and the only present value passes through. Ditto with apply_first/second, they only apply to entries with stored values.

The upside here is that now it is consistent with scalar application, so you can do w - 5 and 5 - w which will apply_second/first with the MINUS operator with no effect on unstored values.

szarnyasg commented 4 years ago

Indeed, the scalar application works around the counter-intuitiveness of point 2. I think there will be an explicit zero but that's easy to get rid of.

Understood regarding point 1 that you need a monoid/semiring to interpret whether something is actually zero or not.

So the direction of this the issue and the relevant pull request looks good to me.

michelp commented 4 years ago

And you know, the more I think about it, + and * maybe aren't the best map to eWiseAdd and eWiseMult which are really much more like & (union) and | (intersection) operations that just happen to default to plus and times operations and the actual math operations are just nice shortcuts to eadd with fixed operations. Maybe I'm overthinking it.

szarnyasg commented 4 years ago

Yeah, it is quite counter-intuitive. I once wasted an hour on this, and this experience led me to make that slide.

marci543 commented 4 years ago

The current behaviour of __sub__ follows MATLAB as discussed in #27 and one of our conversations.

Although v - w ≡ v + (-w) seems more intuitive for a beginner and more pythonic, I think pygraphblas can follow the GraphBLAS behaviour, in which case the tutorials should explain the correct way of subtraction and emphasize that - in (py)graphblas is nothing more than having the union (∪) of present elements as result and GraphBLAS only applies binary operators if necessary, i.e. if both operands are present.

Related issues which can be closed if the decision is that v - w is v.eadd(w, add_op=MINUS) and operations keep explicit 0s:

P.S. I think you swapped the operators here.

& (union) and | (intersection) operations

michelp commented 4 years ago

As we discussed all math operators are now A.eadd(B) with the appropriate binary operator. I'll address the possibility of union and intersection operators in another issue.