Add SymmetrizedGradientOp and unit tests#903
Conversation
📚 Documentation |
koflera
left a comment
koflera
left a comment
There was a problem hiding this comment.
Looks good to me. I would first merge this and then, once this is merged, get rid of the changes regarding the symmetrized gradient op and the related tests in #904, so that in #904 we can focus only on the example notebook.
What do you think @ckolbPTB, @fzimmermann89?
| class SymmetrizedGradientOp(LinearOperator): | ||
| r"""Symmetrized gradient operator. | ||
|
|
||
| The symmetrized gradient :math:`\mathcal{E}: \mathcal{R}^d \to \mathcal{R}^{d+1}` |
There was a problem hiding this comment.
Maybe it would be helpful to explicitly state that this here is understood to be point-wise for an image, i.e. the R^d or the C^d is, for example, supposed to denote the space of the derivatives. Potentially, one could think of R^d or C^d denoting the space of the vectorized image and yielding some confusion.
|
|
||
| \mathcal{E}v = \frac{1}{2}(\nabla v + (\nabla v)^{\top}) | ||
|
|
||
| where :math:`(\nabla v)^{\top}` denotes the transpose of the Jacobian matrix :math:`\nabla v`. |
There was a problem hiding this comment.
Maybe here, one could provide an example, e.g. to say that v = v(x,y) would correspond to a scalar-valued function v: R^2 --> R, and then the connection between nabla as an operator applied to the whole image and the Jacobian being meant as for each point becomes clearer.
There was a problem hiding this comment.
Yes - this operator if a finite difference approximation of this, as we dont use the Jacobian of a function v, but always a finite diffrerence for \nabla v.
Maybe rephrase to make this more more clear? I would even consider avoiding "Jacobian" here completly, as we here always consider discrete images - and have a Jacobian Operator that operates on functions.
Would it be Ok with you to formulate this first as the finite difference version and then mention that this is the discrete version of the Jacobian?
Could you add a link to the finite difference operator in the docstring?
Also, could @kofler or @trung-vt add a similiar short description to finite difference operator?
There was a problem hiding this comment.
Yes, I also agree with getting rid of the word "Jacobian" here
|
I would prefer to have similiar docstrings for the finite difference operator and the symmetrized gradient op, i.e. write the description here already for the discrete version of it (as we can only apply it to images, not to functions v) Also, would be great to use similar notation in both operators and link from Open @koflera lgtm. your decision if you want to include the finite difference docstring change here or make a tiny seperate PR. I think it would be great if @trung-vt or you could do it. |
@trung-vt I would be super happy if you could also include the suggested changes in the docstring of the finite difference operator in this PR :) |
trung-vt
force-pushed
the
sym_grad_op
branch
3 times, most recently
from
746da2b to
d6d6c11
Compare
|
Thank you for your suggestions! I have updated the docstrings for both classes (for convenience, I attached the renderings below) 
|
|
Mh, this looks nice and mathematical, but I am not convinced that the average user of mrpro will understand it. Same for "Assuming unit spaceing", "for a continous function" ... etc Can we dumb it down? For an average undergrad phycicist/SWE? |
I wouldn't actually mind being more precise here. But if we have to make it simpler, we could perhaps for the Finite differences operator just keep the following formula (without the h) and say something like: "The finite differences operator implements this operation for the entire tensor for all dimensions i indexed by dim by means of a separable convolution with appropriate filters, where x is a coordinate on the equispacced grid implicitly defined by the tensor. How the boundaries are handled is defined by pad_mode." For the symmetrized gradient, we could then perhaps just keep the formula What do you think @fzimmermann89 @trung-vt? |
trung-vt
force-pushed
the
sym_grad_op
branch
from
a4e3705 to
0adb4ba
Compare
|
Thank you for your comments! Would this be a more straightforward approach? 
|
|
What's the status on this? i think it would be nice to merge this. to me the docs look good as they are now, but they also looked good before |
|
Is the idea to do #904 afterwards also including a TGV reconstruction algorithm like we have for TV? |
yes, I think it makes sense to split the two things. so that we can start using TGV and have the example at a later point. |
(#904 adds to this PR with an example notebook using
SymmetrizedGradientOpfor TGV image reconstruction.)Summary
Add
SymmetrizedGradientOpas a linear operator, and unit tests for correctness and adjointness.Motivation
The symmetrized gradient is required for total generalised variation (TGV) type regularisation, in particular for
second-order TGV as in Bredies (2014). Having a dedicated operator in
mrpromakes it easier to implement TGV-basedreconstruction methods in a consistent way with the rest of the library.
Changes
SymmetrizedGradientOpinmrpro.operatorsFiniteDifferenceOpand change default value ofmodetoforward.Testing
New tests
tests/operators/test_symmetrized_gradient_op.py. All tests pass locally.Notes
No breaking changes expected; existing APIs remain unchanged.
Note
Introduce SymmetrizedGradientOp with forward/adjoint implementations, export it, and add comprehensive unit tests including CUDA coverage.
SymmetrizedGradientOp: Implements symmetrized gradient as0.5 * (1 + transpose) @ FiniteDifferenceOp; supportsdim,mode={'central','forward','backward'}, andpad_mode={'zeros','circular'}; providesforwardandadjoint.mrpro.operators.__init__and added to__all__.tests/operators/test_symmetrized_gradient_op.py):dim/mode/pad_modesettings.Written by Cursor Bugbot for commit 7faa03d. This will update automatically on new commits. Configure here.