Algorithm.run(callbacks: list[Callback])

[casperdcl]

Describe your changes

"Oh how the scope hath crept"
the new reviewers wept.
What started small
is now a sizeable overhaul –
though not all suggestions were kept.
– @casperdcl 2024-02-01

• add utilities.callbacks (after utilities was created in Stochastic sampler only  #1642)
• deprecate Algorithm.run(callback: Callable) in favour of Algorithm.run(callbacks: list[Callback])
• ✅ Callback (abstract base class)
  • ✅ _OldCallback (to wrap old-style def callback(iteration, objective, x))
  • ✅ ProgressCallback based on tqdm.auto
    • ✅ TextProgressCallback prints to screen on separate lines every update_objective_interval
      • ✅ LogfileCallback prints to file
  • ❓ EarlyStoppingCallback works by doing raise StopIteration - only added test & basic docstring mention for this, no full example.... because inheriting & overriding Algorithm.should_stop is probably better design in most cases, and also because a concrete EarlyStoppingCallback use-case would be task-specific so deserves to be in CIL-Demos instead.
• misc tidy
  • whitespace (diff without whitespace)
  • drop python 2 syntax
• drop Algorithm(**kwargs) to raise errors on unsupported options (part of Use of **kwargs in methods and functions signature is confusing #1115)
• soft deprecate max_iteration:

old behaviour:

1. Algorithm().run(): TypeError(None)
2. Algorithm(max_iteration=10).run(): 10/10
3. Algorithm().run(iterations=5): 5/5
4. Algorithm(max_iteration=10).run(iterations=5): 5/5
5. Algorithm(max_iteration=5).run(iterations=10): 10/10

new behaviour:

• (1) does 1/1 but prints a warning
• (2), (4), and (5) behave the "old" way but print warnings
• (3) Algorithm().run(iterations) is the only recommended way now.
  • algo.run(5); alg.run(4) will do 5/5 followed by 9/9
• deprecate Algorithm(max_iteration)
• require Algorithm.run(iterations)
• run logic:
  • current = self.iteration
  • total = self.iteration + iterations
  • for step in range(current, total): progress = step/total * 100

Describe any testing you have performed

• added test_algorithms.TestCallbacks
• ran https://github.com/TomographicImaging/CIL-Demos/blob/main/demos/2_Iterative/01_optimisation_gd_fista.ipynb:

default behaviour	extra TextProgressCallback(miniters=5)

Link relevant issues

• fixes Callback in Algorithm #1584

Checklist when you are ready to request a review

• I have performed a self-review of my code
• I have added docstrings in line with the guidance in the developer guide
• I have implemented unit tests that cover any new or modified functionality
• CHANGELOG.md has been updated with any functionality change
• Request review from all relevant developers
• Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

• The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License.
• I confirm that the contribution does not violate any intellectual property rights of third parties

[epapoutsellis]

Since you have a base Callback, it would be nice to implement the following 3 classes that inherit from Callback and do sth with X, where X can be anything...

1. Computing class
2. Storing class
3. Printing class

Although there are no classes atm for 1)-3), we use them in the Algorithm class for .objective and timing for example. They will improve the algorithm class and can also act as templates for user and devs to define their own callbacks to compute different things.

For example a) Compute SSIM given a reference image, b) Compute PSNR for a ROI, c) Compute $||x_{k+1} - x_{k}||_{2}$ and raise StopIteration if is below $\varepsilon$. For c) you need to override .should_stop method.

Users can have access to a), b), c) similar to .objective and timing as myGD_LS.ssim, myGD_LS.psnr or myGD_LS.l2_norm_of_consecutive_iterates. The name of the attributes can be defined by the users. They can also choose to print them or not. Also, we can give them freedom on the the frequency or update_objective_interval.

Finally, there is a problem with .objective and .timing. .timing measures the .update method. So at the end of .run our GD took np.sum(myGD.timing) which is true. However, in practice the user will wait more than np.sum(myGD.timing) and this depends on the update_objective_interval. You will not see a difference for a 2D or small/medium ImageData or AcquisitionData but for large 3D data you will notice a significant difference...

[casperdcl]

1. Computing class
   a) Compute SSIM given a reference image
   b) Compute PSNR for a ROI
   c) Compute $||x_{k+1} - x_{k}||_{2}$ and raise StopIteration if is below $\varepsilon$
2. Storing class

Nice ideas... we can do this in follow-up issues/PRs (also note that some of what you describe should be done by subclassing Algorithm rather than Callback)

3. Printing class

added TextProgressCallback - please do check if this does what you expect @paskino @epapoutsellis :)

there is a problem with .objective and .timing

Sounds like an unrelated follow-up issue?

[MargaretDuff]

Looks good Casper - neatens things up and I think that it will be a great increase in functionality. I highlighted a few mostly doc-string changes. We also need to think about what changes are required in the optimisation.rst file for the documentation

[paskino]

added TextProgressCallback - please do check if this does what you expect @paskino @epapoutsellis :)

Would you be able to share an output example?

[casperdcl]

Would you be able to share an output example?

added another screenshot to the PR description

[casperdcl]

thanks @MargaretDuff - I switched to "new-style" docstrings in the new callbacks.py file, but kinda feel updating existing Algorithm.py should be in a separate PR.

[paskino]

The current verbose setting in run will output something like

Iter   Max Iter  Time(s)/Iter            Objective
        0         20         0.000          1.79200e+03
        5         20         2.314          4.05011e+06
       10         20         2.256          4.17628e+06
       15         20         2.296          4.18019e+06
       20         20         2.347          4.18034e+06
-------------------------------------------------------
Stop criterion has been reached.

This output depends on 3 parameters:

• update_objective_interval which is a member of the algorithm instance telling every how many iteration the objective function needs to be evaluated.
• print_interval controls every how many iterations we make a print. In the example above it is equal to the update_objective_interval
• verbose which controls the verbosity level. Basically the PDHG and SPDHG algorithms can output the primal objective value, the dual objective value and their difference.

I see a couple of issues with the proposed new default and TextProgressCallback:

• the maximum of the progress bar is currently using the algorithm instance member parameter max_iteration. IMHO run should use the iterations parameter as the end of progress bar.
• we want to print by default only every update_objective_interval or print_interval if specified. I may be convinced that printing every ~5 sec is better
• The old print to screen was more orderly than the proposed implementation

[MargaretDuff]

Following on from @paskino's comment. I am concerned that with the current default in "TextProgressCallback" the objective is only updated every update_objective_interval but it is outputted every ~5seconds. Thus, if 5seconds < time it takes for update_objective_interval iterations, the user will see their objective remain the same. They may think this means convergence or that their algorithm is not working. The defaults, in particular, need to avoid this scenario.

[casperdcl]

• Ah I think I finally understood about the update_objective_interval/print_interval logic... try now? I updated the screenshot in the description again
  • default miniters=update_objective_interval
  • dropped all the mininterval/seconds-based stuff
• for pbar.total, using run(iterations: int) rather than algorithm.max_iteration: this isn't really possible
  • callbacks don't - and shouldn't - have access to run() args
    • you can explicitly pass in args algorithm.run(iterations=N, callbacks=[ProgressCallback(total=N)])
  • the "normal" use-case should be to leave the default iterations=None so that max_iteration is used anyway
  • user-overriding algorithm.max_iteration with run(iterations) IMO indicates a partial/debugging/early-stopping run so should indeed show an incomplete progress output
• "old print to screen was more orderly": do you mean you want a tabulated output?

[paskino]

• "old print to screen was more orderly": do you mean you want a tabulated output?

That'd be great, I think.

[MargaretDuff]

Ah I think I finally understood about the update_objective_interval/print_interval logic... try now? I updated the screenshot in the description again

• default miniters=update_objective_interval
• dropped all the mininterval/seconds-based stuff

Thanks @casperdcl! That looks good to me

[casperdcl]

using bar_format="{n:>6d}/{total_fmt:<6} {rate_fmt:>9}{postfix}" "orderly" enough now @paskino?

[MargaretDuff]

We also need to think about what changes are required in the optimisation.rst file to document the increased functionality

[casperdcl]

fyi since I purged Algorithm(**kwargs), @epapoutsellis @gfardell @paskino please check 29d52fa where I removed an apparently unused GD(rate).

[MargaretDuff]

fyi since I purged Algorithm(**kwargs), @epapoutsellis @gfardell @paskino please check 29d52fa where I removed an apparently unused GD(rate).

I think rate=step_size - presume it was missed in a find and replace :)