-
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
Support RayTransform
custom backends
#1540
Conversation
Checking updated PR... No PEP8 issues. Comment last updated at 2020-04-16 18:24:43 UTC |
d9fb3f1
to
ca63f84
Compare
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.
Hi @adriaangraas. Thanks a lot for your contribution! I'm always in favor of making code more extensible and better structured. Your suggestion for a standardized ray transform interface is certainly interesting.
What I definitely like is the single class for forward and adjoint, since they share a lot of logic anyhow.
Maybe it's just me, but I've become a bit suspicious to class hierarchies recently, and this proposal solves the problem of unifying an interface by adding a base class layer to the implementations. That's a bit of a red flag to me and makes me wonder if there's no other way.
Regarding the extra methods like can_handle_size
, I'm also not really sure if it's worth the trouble. Every additional method raises the threshold of implementing a subclass, even if the methods are optional. And if they're only used internally, why adding them? For instance, the check with the size warning could simply be in the __init__
of the implementation class. Who else would be interested?
I'm wondering what we want to achieve here? I guess the goal is to offer a point of extension (other than a PR) to users of the library who have their own ray transform implementation. It means that RayTransform
should have some kind of lookup table for implementations that external users can amend at runtime. The basic tasks of an implementation are
- initialization including sanity checks (default can be provided),
- calls forward and backward,
- (if necessary) managing of external objects (like ASTRA IDs)
- (optional) caching
So in principle I'd expect that for an impl subclass, it would suffice to provide call_forward
and call_backward
. Would that be possible here?
Besides, I think we can easily scrap the RayTransformBase
class in favor of an inline adjoint
class. I know that was discussed at some point, and it was argued that RayBackProjection
should be an operator on its own. But I've yet to see a case where it's insufficient or less intuitive to create a RayTransform
and then take its adjoint
.
Hi @kohr-h, thanks for reviewing! I think you have fair points here. Although I like programming to interfaces (more to open the class for extension, not necessarily to standardize), I agree that OOP machinery easily ends up with a lot of boilerplate code. I tried removing the I also removed the I've two direct questions:
|
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.
Thanks for the update @adriaangraas! I like the structure already a lot better than in the first iteration. Flattening the hierarchy definitely helped to clarify the logic. Some detail comments below (plus some nits about formatting and stuff 😉 ).
I'm not sure how you'd design the inner
adjoint
class, but I thought that a very natural way was to composeRayBackProjection
using instead of extending aRayTransform
. I gave this a try as well, let me know what you think.
A nice example of how this can work is here:
odl/odl/solvers/functional/functional.py
Lines 1236 to 1261 in 0d06b07
def gradient(self): | |
r"""Gradient operator of the functional. | |
Notes | |
----- | |
The derivative is computed using the quotient rule: | |
.. math:: | |
[\nabla (f / g)](p) = (g(p) [\nabla f](p) - | |
f(p) [\nabla g](p)) / g(p)^2 | |
""" | |
func = self | |
class FunctionalQuotientGradient(Operator): | |
"""Functional representing the gradient of ``f(.) / g(.)``.""" | |
def _call(self, x): | |
"""Apply the functional to the given point.""" | |
dividendx = func.dividend(x) | |
divisorx = func.divisor(x) | |
return ((1 / divisorx) * func.dividend.gradient(x) + | |
(- dividendx / divisorx**2) * func.divisor.gradient(x)) | |
return FunctionalQuotientGradient(self.domain, self.domain, | |
linear=False) |
The class is just defined inline, and the outer
self
is bound to a different name so that it's not shadowed. As a bonus, the inner class doesn't even need a dedicated __init__
since it can just use the one of the superclass Operator
.
- Now that I moved warnings into the implementations I cannot suppress the
size
warning, like here. I could either add averbose
flag, or try to catch the warnings, but neither seem very appealing. Do you have a good solution here?
Did I mention that I don't really like the warnings at all?
One way I can think of would be to restrict the automatic selection to fast backends and force users to explicitly choose slow ones. In that case the warnings would be obsolete since slow backends are opt-in. @adler-j ?
- I used a
complexify
function to turn real-valued functions into complex-valued functions. Is there a nicer way, maybe using ODL utilities, to achieve this?
Well, my first thought was to translate your construct into a decorator for call_forward
and call_backward
, but that cannot work since the self
argument is needed. What works instead is to make a wrapper function that handles both cases (I haven't tested the code):
def _add_default_complex_impl(fn):
def wrapper(self, x, out, **kwargs):
if self.reco_space.is_real and self.proj_space.is_real:
fn(x, out, **kwargs)
return out
elif self.reco_space.is_complex and self.proj_space.is_complex:
# NB: change calling code so that `out` cannot be `None`
fn(x.real, out.real, **kwargs)
fn(x.imag, out.imag, **kwargs)
return out
else:
raise RuntimeError('mix of real and complex')
return wrapper
@_add_default_complex_impl
def call_forward(self, x, out, **kwargs):
# Code for real version goes here
@_add_default_complex_impl
def call_backward(self, x, out, **kwargs):
# Code for real version goes here
odl/tomo/backends/astra_cpu.py
Outdated
"size. Consider using 'astra_cuda' if your machine has an " | ||
"Nvidia GPU. This warning can be disabled by explicitly " | ||
"setting `impl='astra_cpu'`.", | ||
RuntimeWarning) |
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.
I have to say that I never was a big fan of these warnings. They come from #1303, and you can read the discussion there for the reasoning.
Thanks again @kohr-h, I don't mind the nitty comments 👍 Open remain mostly decisions from the unresolved conversations above. Is there anything else that would be good to add to this PR? About the decorator: Although the syntax is really clean in the class I don't like that it implicitly passes In a next update I'll try to have a look at the docs and tests! |
I'll go through it in detail again to get a better picture. But one thing that comes to my mind is the Do you think that's reasonable? (Or maybe that's already the current status.) |
Hi @kohr-h. Hm, I don't know about auto-registering to the global dictionary, sounds like code that is hard to read and debug. I can see that this allows users to register their own class, which will then automatically be used by Update: I guess we could make |
21b4e4e
to
0dd1836
Compare
That's what I mean. Two things, then: maybe as a public dictionary, the name should be a bit more verbose. And we don't need the two above, |
I've changed |
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.
@adriaangraas I'm currently rebasing on master. There has been a rather big internal API change (#1459) that impacts this PR, so I think it's easiest if I deploy the necessary changes here so you can focus on the actual content of the PR. Is that okay? So don't worry about an upcoming force-push 😉 |
af3069d
to
f952b9e
Compare
- Opening and closing parens on separate lines if line needs split - No more backslash line continuation - Sorted imports - Removal of remaining references to `DiscreteLp`
Okay, I pushed a couple of commits: the rebase, fixes of rebase accidents and some formatting updates. Please take it from here @adriaangraas |
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.
One little thing, and then we're good to go!
And I just did it myself :-) |
Not waiting for CI. The last commit built fine, so I'll just merge it. |
Thanks a lot for all the work @adriaangraas! |
And thanks a ton for all the guidance @kohr-h, I've also learned a lot! |
At this point it is difficult from the user level to specify a different backend for the
RayTransform
operator. (Current backends are Scikit-image, ASTRA CPU and ASTRA CUDA). Loading a custom backend is necessary if you like to modify the way ASTRA is called, for instance.In this PR I propose to move some backend logic out of the
RayTransformBase
andRayTransform
classes and into newRayTransformImplBase
class and implementation classes. Something like this was already in place forimpl='astra_cuda'
. With this PR a user can for instance subclass the ASTRA CPU implementation and feed its type into theimpl
argument (strategy design pattern).Any implementation must subclass
RayTransformImplBase
, essentially forcing thecall_forward()
andcall_backward()
. There are some optional methods in the base class, such assupports_geometry(geom)
andsupports_reco_space()
.Behavior and API for
RayTransform
andRayBackProjection
remains backward compatible, in particular you can still specify'astra_cpu
' etc. toimpl
. However,AstraCudaProjectorImpl
andAstraCudaBackProjectorImpl
are merged intoAstraCudaRayTransformImpl
. A lot of unnecessarily duplicated code has been removed there, and I didn't expect a lot of users to directly rely on these classes.I'll make proper docs and fix the tests :) but I first wanted to send this in for some feedback on the concept.