Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Progress bar callback #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Progress bar callback #78

wants to merge 1 commit into from

Conversation

atsanda
Copy link
Collaborator

@atsanda atsanda commented Mar 21, 2024

  • ProgressMeter dependency was added. If there is a notion of optional dependencies, it could be worth trying, but I'd need a bit of help/advice with that. Changes inside Project.toml are worth reviewing
  • The struct employs lazy initialization as the number of iterations can be set only when a solver is provided

Related issue:
#71

@atsanda atsanda requested a review from nHackel March 21, 2024 08:32
@JakobAsslaender
Copy link
Member

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
Copy link
Collaborator Author

atsanda commented Apr 2, 2024

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
Copy link
Member

nHackel commented Apr 2, 2024

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
Copy link
Collaborator Author

atsanda commented Apr 2, 2024

Thank you @nHackel !
I will do so then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants