Skip to content

Conversation

@eriknw
Copy link
Member

@eriknw eriknw commented Feb 18, 2021

Also, grblas.op.numpy, which remains lazy.

We now have grblas.ops and grblas.op. Perhaps we should rename grblas.ops.

This was @jim22k's idea to help make grblas easier to use.

Also, `grblas.op.numpy`, which remains lazy.

We now have `grblas.ops` and `grblas.op`.  Perhaps we should rename `grblas.ops`.
@eriknw
Copy link
Member Author

eriknw commented Feb 19, 2021

Some methods like build and apply(op, right=1) require a binaryop and don't accept monoids. Having the monoids in grblas.op instead of binaryops may make using these methods more annoying.

Should we always convert monoids to binaryop when appropriate? This should always be safe and possible to do, right? This is similar to upgrading builtin binaryops to monoids when possible in ewise_add.

@jim22k
Copy link
Member

jim22k commented Feb 19, 2021

Given users can grblas.binary.plus and almost never need to access grblas.ops, lets rename it to grblas.operators or something like that.

And yes, auto-converting from monoid to binary or binary to monoid as required by a method seems okay to me.

We do this by adding unsurprising attributes to all our operators.
@eriknw
Copy link
Member Author

eriknw commented Feb 20, 2021

  • Renamed to grblas.ops to grblas.operator.
  • Allow monoids any place where binaryop is required.
  • Allow binaryop where monoids are required if it is part of a monoid.
  • Added attributes such as binop.monoid, monoid.binaryop, and monoid.identity.

All this made this PR much more substantial.

Question: can we not allow using a semiring where a binaryop or monoid is expected? For example, with ewise_add.

min_plus(A | B)  # bad.  does this do min or plus?
min_plus.binary(A | B)  # better
plus(A | B)  # best

min_plus(A & B)  # bad
min_plus.monoid(A & B)  # better
min(A & B)  # best

class TypedOpBase:
def __init__(self, name, type_, return_type, gb_obj, gb_name):
def __init__(self, parent, name, type_, return_type, gb_obj, gb_name):
self.parent = parent
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is .parent the best attribute name for the untyped version?

binary.plus[int].parent is binary.plus

@eriknw
Copy link
Member Author

eriknw commented Feb 20, 2021

I should have said I plan to disallow using semiring where it grabs the binary/monoid from it such as ewise_add. Let me know if you prefer otherwise.

Although this is part of the spec, it's pretty weird to do in Python.
It's also easy to do e.g. `my_semiring.binaryop` if you want the binarop.
self.name = name
self._typed_ops = {}
self.types = {}
self._anonymous = anonymous
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be private or not?

@eriknw
Copy link
Member Author

eriknw commented Feb 21, 2021

Note that we no longer include Monoids in grblas.op. BinaryOps are more general w.r.t. the types they can operate on.

Also, complex monoids still aren't working.
@eriknw
Copy link
Member Author

eriknw commented Feb 21, 2021

@jim22k one more question for you. When users register new operators, we assign them to e.g. grblas.binary.my_udf if they are not anonymous. Should we also save it to e.g. grblas.op.my_udf?

This is mostly because SuiteSparse doesn't like user-defined binops here.
If the user also defined a monoid from the binop, then we can use the monoid instead.
Also, use `GrB` objects when possible over `GxB` objects.  I think this may be
nicer for the recorder.
# np.array(values2, dtype=np.complex128),
# ],
# ]
# )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw @jim22k complex monoids still don't work. I apparently added a check in "operators.py" to avoid the segfault.

@eriknw
Copy link
Member Author

eriknw commented Feb 27, 2021

Well, this "small" PR became quite large. Merging!

@eriknw eriknw merged commit cbe7989 into python-graphblas:main Feb 27, 2021
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.

2 participants