Computation between scalars - #358

coverage: 99.468% (+0.9%) from 98.605%
when pulling 568cd86 on eriknw:scalar_compute
into ec2cad6 on python-graphblas:main.

This is ready for review if anybody wants to take a look. I plan to merge this PR in a week or so if there's no feedback.

I think a useful way to review this would be to try it out and compare to how you expect computation between scalars to behave.

Thanks for looking @jim22k!

There's one thing I almost forgot: docstrings! I will add docstrings to the new methods on Scalar before I merge.

@jim22k, there's one more edge case for Scalar computation that I want to bring attention to.

__eq__, __ne__, __neg__, and __invert__ are all greedy and return results, not expressions. __eq__ and __ne__ return Python bool, and __neg__ and __invert__ return gb.Scalar. This matches existing behavior of scalars. However, these return expressions for vectors and matrices.

All other scalar dunder methods (such as s + t) return expressions. This includes comparators such as s < t.

What should we do for infix and prefix operators for scalars? Should they always return gb.Scalar? Note that with SuiteSparse:GraphBLAS, s("+") << t + u is able to be done with a single GraphBLAS operation:

GrB_Vector_eWiseAdd_BinaryOp((GrB_Vector)s, NULL, GrB_PLUS_FP64, GrB_PLUS_FP64, (GrB_Vector)t, (GrB_Vector)u, NULL);

Also note that empty_s == empty_t currently returns True. Would returning an empty bool scalar make more sense?

In summary, this PR currently matches old behavior to be backwards compatible, and otherwise infix methods return expressions to match with Vector and Matrix. This may be surprising at times, so we should consider being backwards incompatible.

I want to come to consensus on this before merging.

Here's my current position regarding the previous comment:

• Changing behavior of __eq__ and __ne__ would be too disruptive; keep as is and return bools
  • The behavior is also reasonable-ish and probably easy to get used to
  • But I hope we won't regret empty == empty someday
• Other dunder methods should return expressions as is done with vectors and matrices
  • Expressions are value-y enough that things should still "just work" most of the time
  • Allowing other comparators such as s < t to be true, false, or empty is useful
• Change __neg__ and __invert__ to return expressions
  • These aren't special enough to be different
  • It's probably safe to change these behaviors, as they're probably not used that often 🤞
  • I think it would be best to compute the result of these in GraphBLAS instead of in Python

The changes (__neg__, __invert__) sound reasonable, and I agree with keeping current behavior for __eq__ and __ne__. Even the empty == empty makes sense to me. That is the same behavior as calling s1.isequal(s2).

If for some reason we wanted to change the behavior of __eq__ in the future, we have to consider that non-empty == empty would yield an empty Scalar -- at least, that is what happens for Vector and Matrix. Maybe that behavior is useful, but I agree that most times people are actually reaching for .isequal, not element-wise equality.

They also aren't locked out of doing element-wise equality. They would just have to use the non-infix notation, which makes it explicitly clear what is being asked for (i.e. binary.eq(s1 & s2))

Perfect, thanks so much @jim22k for the careful consideration. I agree with everything you said, so sounds like we're in agreement!

Now I just need to add those docstrings...

This is in 🎉 🚀 🎉 !

This ended up being a bigger PR than I expected, but it makes a lot of improvements, and operations with scalars seems pretty nice.

btw, one of the misc things added in this PR is running Pylint sometimes in CI. It is only informational--it doesn't fail the CI job.

Pylint is actually how I learned awkward 2 is out and we needed to update (another misc update done in this PR), so I think it can actually be a useful check. I'm tempted to allow it to fail PRs, but let's do a "dry run" for now to see how it goes.