JuliaImageRecon / RegularizedLeastSquares.jl

MIT License
20 stars 9 forks source link

Progress bar callback #78

Open atsanda opened 5 months ago

atsanda commented 5 months ago

Related issue: https://github.com/JuliaImageRecon/RegularizedLeastSquares.jl/issues/71

JakobAsslaender commented 5 months ago

Thanks, @atsanda for the PR! Can I quickly ask what the advantage of ProgressBar over the already supplied verbose option is? I am trying to weigh the added functionality vs. the added dependency. I personally try to avoid extra dependencies to keep packages lightweight and make maintenance easier. Of course, ProgressBar.jl is a wide-spread and lightweight package, so adding it might be worth it if you (and others) feel that this adds substantial value and that defining the callback on a per-use basis is too much of an overhead. @tknopp, what are your preferences?

atsanda commented 5 months ago

Hi @JakobAsslaender! There was a feature request in the issue I linked. This PR addresses that request. As far as I understand, verbose prints out the internal properties of an optimiser and can be implemented individually for each of them. Callbacks only have access to the result after each iteration, the ProgressBarCallback is only meant to give a visual impression of the duration of the process. As for dependency, this is what I've already pointed out, I fully agree here, and if there is a mechanism in Julia that allows this to be avoided, it would be a great option here. For example, this callback could only be accessible if `ProgressBar.jl' is already installed. However, I'd be grateful for a hint on how to implement this. Overall, if it is not necessary, we can just close it for now along with the issue.

nHackel commented 5 months ago

It is possible to add add the callback only if ProgressMeter is loaded, however since we require a struct for it we need to do a few tricks. These can also be seen in LinearOperatorCollection.

First we need to define an abstract ProgressMeterCallback, similar to for example this FFT op. This is so that we can export the ProgressMeterCallback in RegLS and workaround the fact that package extension are not designed to add new exports of structs and functions.

Then we can add a new folder for our extesion, such as ext/ and in there add the ProgressMeterExtensionExt, similar to the FFT extension. This is a new module that loads both RegularizedLeastSquares and ProgressMeter.

In this module we then define ProgressMeterCallbackImpl.jl, this is the actual struct. There we then have the same code as you previously had in this PR and you implement RegularizedLeastSqaures.ProgressMeterCallback to construct the impl struct, similar to here.

Lastly we need to change the Project.toml. There you need to add a weak dependency as well as an extension, similar to here.

These changes would give us the functionality from before conditionally loaded with ProgressMeter

atsanda commented 5 months ago

Thank you @nHackel ! I will do so then.