funsor icon indicating copy to clipboard operation
funsor copied to clipboard

Strengthen algebraic organization of funsor.ops and related code

Open eb8680 opened this issue 5 years ago • 1 comments

The changes proposed in #304 and the incorrect code path for logaddexp uncovered in #306 suggest adding more structure to the funsor.ops module.

Possible changes:

  • [ ] To address the bug found in #306, centralize enforcement of the contract that op implementations are opaque to Funsor and are only invoked on underlying data objects like torch.Tensors, perhaps in the base definition of ops.Op
  • [ ] As proposed in #304, associate a backward op with each AssociativeOp for argreduce computations so that the backward op arguments in funsor.adjoint methods can be removed
  • [ ] Create a CommutativeOp or CommutativeAssociativeOp class to distinguish between commutative and non-commutative monoids
  • [ ] Create a Semiring data structure for use by Contraction and normalize containing a sum and product op and (where applicable) their units and inverses instead of passing around separate ops sum_op and prod_op and storing units and inverses globally

eb8680 avatar Jan 27 '20 02:01 eb8680

op implementations are opaque to Funsor and are only invoked on ... data

I seems more natural to me to extend Python's builtin operator library by letting e.g. ops.add apply to funsors. That way we can use Python idiomatic syntax to describe both data and funsors. A simple fix for logaddexp is to add .__logaddexp__() method handling as in #306; we did the same thing for .__matmul__ in Python 2 before it was standardized in Python 3. Generally it seems natural to let ops strictly extend operator by (1) adding more structure (as proposed in this issue), and (2) adding more ops.

fritzo avatar Jan 27 '20 18:01 fritzo