Update _linear_operator.py #9
Conversation
Add additional LinearOperator.__add__ case for Pythonic addition operations with the builtin sum() function. After other common use-cases, check if other is a numeric Python type and equal to zero, and if so, return self.
| @@ -2556,6 +2556,8 @@ def __add__(self, other: Union[torch.Tensor, LinearOperator, float]) -> LinearOp | |||
| from .sum_linear_operator import SumLinearOperator | |||
| from .zero_linear_operator import ZeroLinearOperator | |||
|
|
|||
| import numbers | |||
|
|
|||
| if isinstance(other, ZeroLinearOperator): | |||
| return self | |||
There was a problem hiding this comment.
This is tangential to this PR but touches the same function:
Shouldn't we return a copy of self in the ZeroLinearOperator branch?
Generally, out-of-place addition yields a new object in memory. A user could therefore assume that the returned sum is independent of the summands. This seems especially relevant to DenseLinearOperator, whose add method falls back to this generic implementation, but similarly to DiagLinearOperator etc.
E.g. assigning a new value to some of the entries of the returned sum
would cause unintended side-effects.
There was a problem hiding this comment.
Hmm, that seems like a good point to me. In general I think we should try to not assign new values to entries of LinearOperator objects post construction, but I guess that could happen so better be safe than sorry (especially since returning a new ZeroLinearOperator will be cheap). Curious what @gpleiss's thoughts are on this.
There was a problem hiding this comment.
It might be more expensive since self is generally not a ZeroLinearOperator, even though other is.
Also, a few operators have the base_linear_op field, which signals that it is not a private variable, and could be up for mutation.
I introduced the copy here. We can continue the discussion there as to not block @wbeardall in merging this PR.
There was a problem hiding this comment.
It might be more expensive since self is generally not a ZeroLinearOperator, even though other is.
D'oh. Of course.
Also, a few operators have the base_linear_op field, which signals that it is not a private variable, and could be up for mutation.
Hmm I think in general one should not mutate the base_linear_op field outside of the LinearOperator's methods. If that's universally accepted we may want to rename and privatize the constituent operators.
There was a problem hiding this comment.
Hmm I think in general one should not mutate the base_linear_op field outside of the LinearOperator's methods. If that's universally accepted we may want to rename and privatize the constituent operators.
Agreed 👍
Co-authored-by: Max Balandat <[email protected]>
|
@gpleiss We actually had to back out the 'deepcopy' again because it broke things downstream in Botorch. I'd say let's not include that here until we understand how to fix it. |
|
Okay, let's merge this PR as is (well... after fixing linting), and then I'll open an issue so that we don't forget to switch this to deep copy one day. |
Add additional LinearOperator.add case for Pythonic addition operations with the builtin sum() function. After other common use-cases, check if other is a numeric Python type and equal to zero, and if so, return self.
Fixes #8.