-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Linearized deformations second take #488
Conversation
ee2b210
to
e22d8af
Compare
Cleaned up commit history and implementation, also rebased to master. Should be reviewable. |
|
||
return PointwiseInner(self.domain, def_grad) | ||
|
||
def __repr__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repr
is short for representation and is intended to give a representation of the object. It is called when you write things like
>>> space = odl.rn(3)
>>> space
rn(3) # Here repr is called
and can also be explicitly invoked through the repr
function
>>> space = odl.rn(3)
>>> print(repr(space))
rn(3)
You can read some more in the official documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, you should be able to create the object from the repr string, but this is not always possible.
>>> np.array([1, 2, 3, 4, 5])
array([1, 2, 3, 4, 5]) # can recerate the array from this
>>> np.eye(5)
array([[ 1., 0., 0., 0., 0.],
[ 0., 1., 0., 0., 0.],
[ 0., 0., 1., 0., 0.],
[ 0., 0., 0., 1., 0.],
[ 0., 0., 0., 0., 1.]]) # can still recreate the array
>>> np.eye(50)
array([[ 1., 0., 0., ..., 0., 0., 0.],
[ 0., 1., 0., ..., 0., 0., 0.],
[ 0., 0., 1., ..., 0., 0., 0.],
...,
[ 0., 0., 0., ..., 1., 0., 0.],
[ 0., 0., 0., ..., 0., 1., 0.],
[ 0., 0., 0., ..., 0., 0., 1.]]) # array too big, cannot recreate it
ODL generally works the same:
>>> odl.vector([1., 2, 3, 4, 5])
rn(5).element([1.0, 2.0, 3.0, 4.0, 5.0])
>>> odl.vector([1.] * 100)
rn(100).element([1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discr_space = odl.uniform_discr([-1, -1], [1, 1], (100, 100), interp='linear')
template = odl.phantom.cuboid(discr_space, [-0.5, -0.5], [-0.25, 0])
fixed_templ_op = odl.deform.LinDeformFixedTempl(template)
Without explicit __repr__()
>>> fixed_templ_op
LinDeformFixedTempl: ProductSpace(uniform_discr([-1.0, -1.0], [1.0, 1.0], [100, 100],
interp=u'linear'), 2) -> uniform_discr([-1.0, -1.0], [1.0, 1.0], [100, 100], interp=u'linear')
With explicit __repr__()
>>> fixed_templ_op
LinDeformFixedTempl(uniform_discr([-1.0, -1.0], [1.0, 1.0], [100, 100], interp=u'linear')
.element([[0.0, 0.0, 0.0, ..., 0.0, 0.0, 0.0],
[0.0, 0.0, 0.0, ..., 0.0, 0.0, 0.0],
[0.0, 0.0, 0.0, ..., 0.0, 0.0, 0.0],
...,
[0.0, 0.0, 0.0, ..., 0.0, 0.0, 0.0],
[0.0, 0.0, 0.0, ..., 0.0, 0.0, 0.0],
[0.0, 0.0, 0.0, ..., 0.0, 0.0, 0.0]]))
It seems the former is more clear. Maybe I am wrong.
Edit: @adler-j typeset python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you are correct in some sense, it depends on your definition of clear. In python there are two "print" statements, __str__
and __repr__
. __str__
is intended to give a short representation of the object so users know what is meant, while __repr__
should be used for debugging and be more precise.
Here I only implemented __repr__
, mostly because I was lazy. We could want to implement __str__
as well however.
'instance if `domain` is None, got {!r}' | ||
''.format(template)) | ||
|
||
domain = template.space.tangent_space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In differential geometry, one can attach to every point x
of a differentiable manifold a tangent space
. Here we attach a tangent_space
to a space
. It seems using space.tangent_space
is not exact. Probably tangent bundle
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the space is flat they should be equivalent, but sure, the naming here is not perfect. We may need some more thought on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's quite a geometric concept here. How about vector filed space
, short for vf_space
or more explicit name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source for the discussion regarding the notion of tangent space is that ODL lacks a notion of manifold (curved space). In ODL one has either a set or a vector space, the notion of a. manifold does not exist. We have already encountered the need to handle manifolds since data space in tomography is a non-Euclidian manifold. The operations required for points (=tomographic data) on such manifolds could however be rephrased as operations on the underlying coordinate space (products of intervals), ignoring the fact that a single coordinate space is insufficient as an atlas. Now, dealing with deformations requires us yet again to handle points (=vector fields) in curved spaces, i.e. manifolds. One option is to try to recast the operations to the underlying coordinate spaces, e.g. assuming a RKHS. Another is to introduce notions manifold in ODL with a tangent bundle. The latter seems to be quite time consuming though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option, and I think that's what @adler-j was trying to achieve, is to represent the space for the vector fields directly, without having the notion of manifolds. That's of course a bit limiting, as for the manifolds in acquisition geometries, but from my perspective it's a sufficient solution and can be handled quite smoothly with the means we have currently. I don't think we necessarily need RHKS to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well for the fixed template style deformations we need a proper domain for the deformations., and in the fixed deformation case we need to be able to give it as input. Basically we need a name for the space
ProductSpace(space.astype(space.real_dtype), space.ndim)
I picked tangent_space
since it felt natural, but as @chongchenmath noted this may not be future proof since it is only correct for flat spaces.
@kohr-h was really on spot on what we try to achieve here. RKHS has nothing to do with this one and probably shouldn't need to be involved for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we talking about the set of deformations or the set of vector/velocity fields generating deformations? The set of deformations is not a vector space, it is a semi group and a Lie group at best. The set of vector/velocity fields generating deformations is however a vector space, which in some theoretical frameworks can be interpreted as the tangent space to the Lie group of deformations. RKHS was mentioned as an example of how this Lie group could be equipped coordinates. So, what are we trying to name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we talking about the set of deformations or the set of vector/velocity fields generating deformations?
We're talking about "the set of vector/velocity fields generating deformations".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I suggested, one abbreviation of vector field space
is probably a proper choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, something along those lines would be good.
efa5d30
to
eb3a60b
Compare
dc334e9
to
cd5fd85
Compare
---------- | ||
template : `DiscreteLpVector` | ||
Template to be deformed by a displacement field. | ||
displacement : `ProductSpace` element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProductSpace of DiscreteLp element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
'instance, got {!r}'.format(displacement[0])) | ||
|
||
if templ_space is None: | ||
templ_space = displacement[0].space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While equivalent, I feel it would be better to write displacement.space[0]
This is starting to look very good, I only found some minor style issues. We should be able to merge soon. |
---------- | ||
template : `DiscreteLpVector` | ||
Template to be deformed by a displacement field. | ||
displacement : `ProductSpace` of `DiscreteLp` element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be same as below
I added an (approximate) inverse to the fixed-displacement operator since we're using it in the adjoint anyway. Fixed the comments. |
c004719
to
b5a5106
Compare
Cleaned up the commit history a bit, will merge after tests have passed. |
* Add linearized deformation operators * Add tests for the linearized deformations * Add examples of linerized deformations
Examples: - Better text, more informative comments and a more interesting displacement field in the deformation examples linear_deform.py: - Simplified imports - Improved docstrings, mostly adding missing documentation and some reformulations - Mathematical descriptions added to both operators - Removed optional `domain` parameter from the fixed-template operator since it complicates things and doesn't add much. - In the fixed displacement variant, it is needed to be able to support complex template spaces - here the support for implicitly discretizing a displacement field was removed for reasons of simplicity. Further, the parameter is now called `templ_space` since it is both domain and range. - Public properties for the fixed attributes `template` or `displacement` added, resp. lp_discr.py: - `_interp_by_axis` -> `__interp_by_axis` - Go back to old version of real space calculation in `DiscreteLp`. The current one arbitrarily added `self.real_space` to an instance. Tests: - Adapt to code changes
b5a5106
to
b8daded
Compare
Second take at linearized deformations. I've cleaned up most of the implementation and updated the doc, should be ready for final review @chongchenmath, @kohr-h, @ozanoktem .