TomographicImaging / CIL

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

PDHG and new callbacks #1781

Open epapoutsellis opened 7 months ago

epapoutsellis commented 7 months ago

Description

Run the tomography reconstruction binder demo TomographyReconstruction

Environment

23.1.0 g5e4938b5 3.9.19 (main, Mar 21 2024, 12:07:41) 
[Clang 14.0.6 ] darwin

Some problems

Setup and run PDHG

pdhg = PDHG(f = f, g = g, operator = K, sigma = sigma, tau = tau, update_objective_interval = 50) pdhg.run(iterations=500, verbose=2, callbacks=[callbacks.TextProgressCallback()])

<img width="422" alt="Screenshot 2024-04-18 at 09 10 37" src="https://github.com/TomographicImaging/CIL/assets/22398586/a012ac23-4b92-4c2a-a577-8f6b5d940ef5">

- Different precision in objective values between `ProgressCallback` and `TextProgressCallback`
```python
pdhg = PDHG(f = f, g = g, operator = K, sigma = sigma, tau = tau, 
            update_objective_interval = 50)
pdhg.run(iterations=500, verbose=2, callbacks=[callbacks.ProgressCallback(), callbacks.TextProgressCallback()])
Screenshot 2024-04-18 at 09 14 05
MargaretDuff commented 7 months ago

Thanks @epapoutsellis some initial thoughts but I can take another look next week -

  • PDHG with verbose=2 does not print primal-dual gap as before

A callback to print the primal-dual gap would be useful for CIL and shouldn't be difficult to write - I can take a look next week.

  • Different precision in objective values between ProgressCallback and TextProgressCallback

I think this is a question for @casperdcl. But are you referring to the different values for it/s or the 2dp vs3dp for ProgressCallback and TextProgressCallback?

  • There is no timing attribute in PDHG or Algorithm class. This used to return a list with times (per update_objective_interval) without measuring computational time of the objectives.

Again, it should be possible to write a callback for this. What timings are you interested in? Being able to time each iteration, excluding the objective calculation?

  • What does it mean 13.57it/s? For some stochastic algs, it would be nice to get the time per one iteration when a stochastic gradient is used and the time per one iteration when full gradient iscomputed.

13.57it/s is the number of iterations per second so the reciprocal of time per iteration.

epapoutsellis commented 7 months ago

Since we compute primal-dual gap by default, I think is better to print it (if one wants) as we did before.

Also, it is strange to see a continuous representation for iterations, e.g., 13.57 iterations. Seconds per iteration is more reasonable which is equal to Seconds per execution time for the update method. Why in ProgressCallback we get 11.57it/s and with TextProgressCallback 13.01 it/s?

We can do many things with callbacks but previous default behaviour was much better. Also, printing in jupyter is very strange.