BUG: Correct OperatorRightVectorMult.derivative, closes #1330. #1331
Conversation
| @@ -2165,7 +2165,7 @@ def derivative(self, x): | |||
| if self.is_linear: | |||
| return self | |||
| else: | |||
| return self.operator.derivative(x) * self.vector | |||
| return self.operator.derivative(self.vector * x) * self.vector | |||
There was a problem hiding this comment.
Apparently I wasn't fast enough :-)
Please first check if we have to multiply. This is a hot code path and we need to do our best to reduce wasted effort.
There was a problem hiding this comment.
I improved this by not checking for linearity, but if the derivative coincides with the operator itself. I checked that it works for odl.RealPart, and all the tests pass.
There was a problem hiding this comment.
Hm, this was my first thought as well, but I discarded it because of the potential extra cost to generate the derivative twice.
We need to be a bit careful with this since derivative(x) may actually do some potentially expensive computation, and then the benefit from doing the check for some operators like RealPart may not outweigh the cost of having to generate the derivative twice for other nonlinear ones.
Opinions, @aringh, @adler-j, @olivierverdier?
There was a problem hiding this comment.
After discussion with @olivierverdier: What about setting the linear flag for the RealPart to True?
There was a problem hiding this comment.
That would also be okay for me, it's the more practical approach anyway. In that case, please revert the changes I made in ce89155 as well, and the part of the documentation mentioning this trade-off.
There was a problem hiding this comment.
I hope, I got everything. Since the linear flag was used for other stuff in the RealPart and ImagPart operators, I introduced some additional flag for them.
| @@ -238,6 +238,6 @@ but | |||
|
|
|||
| Therefore, ODL takes the following pragmatic approach for complex <-> real operators: | |||
|
|
|||
| - Derivatives are taken in the real sense, but linearity is always set to `False` since this refers to linearity in the sense of the involved vector spaces. | |||
| - Even for formally non-linear derivative operators, an adjoint can be defined, which will also not be linear. | |||
| - Derivatives are taken in the real sense. Linearity is set to `True` for an operator :math:`A: X \to Y` if :math:`A'(x) = A` for all :math:`x\in X`. This property can be used to optimize calculations with derivatives, since the derivative operator does not depend on the point. Linearity in the sense of complex vector spaces is currently not reflected by any flag in ODL. | |||
There was a problem hiding this comment.
Please put the sentences on one line each, RST will still output it as a paragraph. The reason is that it works better with Git's line based content tracking.
|
I have not found any false statements in RealPart and ImagPart. However, I was missing an operator like ComplexConj, which would also belong to the category mentioned here. |
|
Yes, I'll merge this in the meanwhile, LGTM |
No description provided.