Unimplemented function in regulariser plugins are opaque

[ashgillman]

There are a number of unimplemented functions in regularisers.py. The following in non-exhaustive.

These have comments:

https://github.com/vais-ral/CCPi-Framework/blob/1803cbd445c408588fecbf705fb8b4df486029fc/Wrappers/Python/cil/plugins/ccpi_regularisation/functions/regularisers.py#L164-L166

https://github.com/vais-ral/CCPi-Framework/blob/1803cbd445c408588fecbf705fb8b4df486029fc/Wrappers/Python/cil/plugins/ccpi_regularisation/functions/regularisers.py#L261-L263

This one prints a warning:

https://github.com/vais-ral/CCPi-Framework/blob/1803cbd445c408588fecbf705fb8b4df486029fc/Wrappers/Python/cil/plugins/ccpi_regularisation/functions/regularisers.py#L184-L188

This one is unmarked:

https://github.com/vais-ral/CCPi-Framework/blob/1803cbd445c408588fecbf705fb8b4df486029fc/Wrappers/Python/cil/plugins/ccpi_regularisation/functions/regularisers.py#L109-L110

They all return 0.

It is understandable for them to be unimplemented, but it would be nice for it to be a little more transparent to the user. Would it make sense to (a) give them all a warning message, and (b) have them return NaN so that the code can run on, but that the results will clearly be invalid when the user attempts to plot them?

[paskino]

Thanks @ashgillman, I'm generally inclined to raise an error rather than returning nan or similar. Here, instead we return 0 which is even wronger.

[ashgillman]

An error makes the most sense if calling these is dangerous (i.e., if it turns out that these may be causing some errors I am seeing in other discussions).

I'm not quite familiar enough with the optimisers to know whether in some cases, ignoring these operators is semi-valid (I assumed that might be the case and why return 0 was included in the first place?). In this case, the warning might be okay, but if there was a warning and no error than I might have thought NaN would be still be more appropriate than 0.

[ashgillman]

Somewhat related, I don't think FGP_dTV.__call__ is correct, this seems to be TV not dTV.

https://github.com/vais-ral/CCPi-Framework/blob/1803cbd445c408588fecbf705fb8b4df486029fc/Wrappers/Python/cil/plugins/ccpi_regularisation/functions/regularisers.py#L229-L232

[paskino]

I believe that the regularisers are meant to be used in our optimisation framework only with their proximal method, which in itself is a minimisation problem. That is the reason why they are functions where only proximal is developed.

For TV and dTV that expression of __call__ might be correct but it's better to ask a mathematician, @jakobsj @epapoutsellis .

[jakobsj]

Any regulariser exposed by the plugin should have a correct __call__ method provided by the plugin, so that for example objective function value is reported correctly in optimisation algorithms. If that is not the case, it must be fixed.

…

On Wed, 10 Feb 2021, 15:19 Edoardo Pasca, ***@***.***> wrote: I believe that the regularisers are meant to be used in our optimisation framework *only* with their proximal method, which in itself is a minimisation problem. That is the reason why they are functions where only proximal is developed. For TV and dTV that expression of __call__ might be correct but it's better to ask a mathematician, @jakobsj <https://github.com/jakobsj> @epapoutsellis <https://github.com/epapoutsellis> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/vais-ral/CCPi-Framework/issues/755#issuecomment-776736417>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACMDSCF7YEL6OYKNOU4UIK3S6KIWXANCNFSM4XIVCUUQ> .

[paskino]

From @epapoutsellis
https://github.com/vais-ral/CCPi-Framework/issues/758#issue-805671962

All of the regularisers that are not TV have a wrong implementations of the __call__ method.

To start with I suggest we remove the definition (override) in the derived class. If no implementation is given the base class method will be called, which already raises NotImplementedError.
Then we can develop the appropriate evaluation of the regulariser Function, but at least the user will not be tricked by getting a wrong value.

Currently, dTV implementation will be implemented in TomographicImaging/CCPi-Regularisation-Toolkit#159

[epapoutsellis]

If we use raise NotImplementedError then none of the algorithms will run because objective at the iteration=0 must be computed. I suggest to return 0. plus a warning message as a quick fix.

[ashgillman]

You could also throw an error, and then catch it at a known place where it is called but the value isn't necessary

[jakobsj]

I believe at this point a value needs to be returned for the algorithms to run as mentioned by @epapoutsellis. For a quick-fix I think that returning nan as suggested by @ashgillman is safer than 0, and is the best option at this point, since it clearly indicates that something is not right, but does allow the algorithm to run. Probably with a warning, but a warning may easily be overlooked which is why returning 0 is dangerous.

[paskino]

Agreement is:

1. return numpy.nan
2. issue a warnings.warn
3. Change the message to be more intelligible to users.

[gfardell]

Fixed in current version