[SyneRBI/SIRF] resampler.process deprecation (#700)

[Kris Thielemans]

SIRF-exercises registration demo does

resampler = Reg.NiftyResample()

resampler.set_reference_image(ref)

resampler.set_floating_image(flo)

resampler.add_transformation(tm)

resampler.set_padding_value(0)

resampler.set_interpolation_type_to_linear()

resampler.process()

but gives warning

/usr/local/lib/python2.7/dist-packages/ipykernel_launcher.py:9: DeprecatedWarning: process is deprecated as of 2.1.0. Use forward(image) instead

  if __name__ == '__main__':

Aide from the fact that Python/jupyter could work on their user-friendliness of their errors (I'm sure @AnderBiguri would agree 😉), my question is if we should have deprecated this.

For registration people, the original syntax is straghtforward (you set-up the registration that way, so you do the same for the resampler). The forward is slightly less easy to grasp in this context.

I'm not sure about what the best is. @rijobro @ckolbPTB ?

Of course, ideally we don't have this warning in the SIRF-exercises, but first we need to know what to do.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[Richard Brown]

Hum, two possible solutions:

1. Un-deprecate the process methods
2. In this example, replace process() with forward(flo).
3. See bottom

Personally, between the first two, I'm in favour of the second. As you say, process might be an easier concept to grasp than forward, and mirrors the registration nicely. However, it will only lead to more confusion if later down the line, the user needs to think about backwards/adjoints and inverses. Also, we would have to put in the documentation of the process method that it only works in the forward direction, which means they'll have to think about what forward and backward mean anyway.

I suppose we could change the signature of process to process(direction='forward') (which obviously also accepts process(backward)).

[Biguri]

@rijobro just nitpicking: process does not only work for forward right? It may not return the adjoint(reg) but it does return the DVFs to do so, am I correct?

[Kris Thielemans]

@AnderBiguri this is the resampler. it doesn't return a DVF (that's the job of the registration)

[Richard Brown]

Not sure what you mean, can you give an example? The DVFs are the same regardless of forward/adjoint.

Anyway, I'm fairly sure that under the hood, process simply calls forward.

[Biguri]

@KrisThielemans @rijobro let me go and buy stronger coffee, completely misread all this. Sorry!

[Richard Brown]

https://github.com/SyneRBI/SIRF/blob/225d6211a8e6adc02d859978764d062a70f36806/src/Registration/cReg/NiftyResample.cpp#L190-L194

[Kris Thielemans]

Obviously, all this forward backward terminology is incredibly confusing in registration. (Most people will think that backward is the inverse of course). Not much we can do about that, except for documenting it, e.g. in the UserGuide (if anyone reads that, but at least we can point them to it). People will have to think about it.

The process() terminology was suggested by @ckolbPTB as a generic method, I'd rather not drop it at this stage. See #710

As I don't really want to create a lot of work now without proper discussion, I suggest that we undeprecate process(), document that it calls forward on the floating.

I see that the UserGuide is out-of-date on the methods for the resampling in any case.

[Richard Brown]

PR to un-deprecate process and improve doc in UserGuide.

For what it's worth, I think this is a pretty good warning!

DeprecatedWarning: process is deprecated as of 2.1.0. Use forward(image) instead

[Kris Thielemans]

Sure, that warning is crystal clear. It's the cruft around it that more distracting than anything else. Anyway. not our issue.

[Kris Thielemans]

Closed #700 via #702.