CIL icon indicating copy to clipboard operation
CIL copied to clipboard

Change in behaviour of PDHG

Open paskino opened this issue 1 year ago • 4 comments

Description

From @samtygier-stfc on Discord

I am upgrading from 23 -> 24, and came across some changes in the way we iterate the PDHG algorithm. Previously we created the PDHG instance with a large number for max_iterations, and then had our own loop to which called next() to run each iteration

algo = PDGH(..., max_iterations=1000000, ...)
for i in range(N):
    algo.next()
    ...

In 24.0 the next() method is gone, but I can do next(algo) instead. Also max_iterations is deprecated in the constructor, with the suggestion to use Algorithm.run(iterations). If I remove max_iteration from the constructor, then next() will give StopIteration straight away. I can get around the deprecation warning by doing

algo = PDGH(...)
algo.max_iterations = 1000000
for i in range(N):
    next(algo)
    ...

paskino avatar Apr 30 '24 08:04 paskino

So far algo.max_iteration is used, although we would like to discourage it in favour of the algo.run method with callbacks. Also, next has been removed (it was there for compatibility with Python2) but __next__ is still there.

paskino avatar Apr 30 '24 08:04 paskino

For now you could use:

algo = PDGH(...)
algo.max_iterations = 1000000
for i in iter(algo):
    ...  # do something with algo

but it might stop working in a future release.

Best is:

from cil.optimisation.utilities.callbacks import Callback, ProgressCallback

class EachIterCallback(Callback):
    def __call__(self, algo):
        ...  # do something with algo

algo = PDHG(...)
algo.run(1000000, callbacks=[ProgressCallback(), EachIterCallback()])

vis. https://tomographicimaging.github.io/CIL/v24.0.0/optimisation/#callbacks

If you've made any custom callbacks you think others will benefit from (e.g. TensorboardCallback or something) please do open a PR adding it to Wrappers/Python/cil/optimisation/utilities/callbacks.py!

casperdcl avatar Apr 30 '24 08:04 casperdcl

I've switched to doing the progress bar updates from a callback: https://github.com/mantidproject/mantidimaging/pull/2184/commits/5006fd30d5ae81a89c7904d32af9af71cb175585

It would be good if there was a mention in the release notes about the change.

samtygier-stfc avatar Apr 30 '24 12:04 samtygier-stfc

I've switched to doing the progress bar updates from a callback: mantidproject/mantidimaging@5006fd3

lgtm!

It would be good if there was a mention in the release notes about the change.

Unfortunately it is mentioned in the highly inaccessible CIL@master:/CHANGELOG.md but for some unknown reason @TomographicImaging/cil-developers seem to dislike keeping the more standard CIL/releases readable/helpful/neat/up-to-date/in-sync 😠

casperdcl avatar Apr 30 '24 16:04 casperdcl