Skip to content

Replace Transform.__add__ with Transform.__matmul__? #17230

Closed
@brunobeltran

Description

@brunobeltran

Bug report

Bug summary

While working on fixing the transform pipeline for image rendering, it struck me that the operator "+" probably sends the wrong impression to users in terms of expected properties. The Euclidean group is barely associative, and definitely not commutative.

Also, since a Transform is something that's applied (like an operator), my expectation as I'm using composite transforms is that they will be applied rightmost-first. In other words, I keep expecting

>>> (A + B).transform(X) == A.transform(B.transform(X))  # but it's currently False
>>> (A + B).transform(X) == B.transform(A.transform(X))  # how it currently works.

Now that Python has the "@" operator, this feels like a more natural choice for "combining" Transforms. People don't generally expect A@B == B@A, while they do expect A + B == B + A, which is definitely not true for Transforms.

The implementation would just be something like (modulo deprecation code)

def __matmul__(self, other):
    return other + self

And we would get

>>> (A @ B).transform(X) == A.transform(B.transform(X))  # returns True

Release strategy

Fixing all our internal uses is (probably) only one PR's worth of work, since (relative to how much code would churn) it would be easy to verify/review.

However, to me this feels like an integral enough part of the Transform API that it might make sense to leave in "+" until 4.x, and maybe just make sure we only advertise "@" in e.g. the tutorials?

In summary:

Pros

  1. "@" implies lack of commutivity, whereas "+" implied commutivity
  2. "@" implies order of application (rightmost-first), whereas "+" is ambigious

Cons

  1. Pretty core part of Transform API
  2. Lots of code-churn in Transform-related internals.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions