TomographicImaging / CIL

A versatile python framework for tomographic imaging
https://tomographicimaging.github.io/CIL/
Apache License 2.0
95 stars 41 forks source link

CGLS - needs either clearer documentation on tolerances or for tolerances to be deprecated #1891

Closed MargaretDuff closed 2 months ago

MargaretDuff commented 2 months ago

Description

There is no mention of a default tolerance here: https://github.com/TomographicImaging/CIL/blob/eb254e0bae77e193f05acc03b6e94cd1c189e4ab/Wrappers/Python/cil/optimisation/algorithms/CGLS.py#L28-L47

We could also do this with a callback:

https://github.com/TomographicImaging/CIL/blob/eb254e0bae77e193f05acc03b6e94cd1c189e4ab/Wrappers/Python/cil/optimisation/algorithms/CGLS.py#L125-L135

MargaretDuff commented 2 months ago

So we can't move the tolerance check to a callback, as that is passed to the 'run' argument and not in the algorithm set-up.

The next question is: should early stopping, based on a tolerance be a default for CGLS or should it be optional and implemented with a callback, like for the rest of the algorithm class?

As CGLS is the only algorithm with a default tolerance, my preference would be to remove it.

@jakobsj @paskino @epapoutsellis @gfardell - do you have any thoughts?

jakobsj commented 2 months ago

Good points. The points you mention convince me that removing the tolerance is the best choice of action - to be more consistent with other algorithms, and because I think the tolerance is chosen somewhat arbitrarily in the very early days. As a callback it will be better and more transparantly controlled by the user. We should just make sure that clear documentation is available on using callbacks and the users only seeing eg docstring is directed toward such.

If I recall the issue of converging in n iterations applies only in infinite precision computations, which we don't have, and also typically the matrix size would be so large that we wouldn't want to run as many iterations, and so the risk of wasting computational effort is negligible.