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

Add a progress bar based on ProgressMeter.jl #136

Merged
merged 6 commits into from
Sep 10, 2020
Merged

Add a progress bar based on ProgressMeter.jl #136

merged 6 commits into from
Sep 10, 2020

Conversation

DilumAluthge
Copy link
Contributor

@DilumAluthge DilumAluthge commented Aug 30, 2020

Fixes #135

This pull request adds the ProgressMeterReport, which uses the ProgressMeter.jl package to display a progress bar.

To use this reporter, simply set the reporter keyword argument when calling mcmc_with_warmup:

reporter = ProgressMeterReport()

cc: @tpapp

@DilumAluthge
Copy link
Contributor Author

Alright, this is ready for review now @tpapp

@tpapp
Copy link
Owner

tpapp commented Sep 4, 2020

Thanks. I am very busy now but will try to do it soon. Feel free to bump this if I don't get to it within a week.

src/reporting.jl Outdated Show resolved Hide resolved
src/reporting.jl Outdated Show resolved Hide resolved
src/reporting.jl Show resolved Hide resolved
Copy link
Owner

@tpapp tpapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I just requested some trivial changes. Thanks for adding this nice feature!

@DilumAluthge
Copy link
Contributor Author

I think I've addressed all the changes!

You will also notice that I have now removed the log_warmup field. This bears some explanation.

Right now, we have separate progress bars for warmup and MCMC. Long-term, I think the best solution will be to figure out how to have a single progress bar for both warmup and MCMC. This will require some more info, e.g. #141

Once we implement that feature, the log_warmup field will no longer make sense. If we add the log_warmup field now, then later, when we are ready to remove it, that would be a breaking change.

So I think it's best to right now remove the log_warmup field, and always generate progress bars for both warmup and MCMC. And then in the future we can work on combining the progress bars for warmup and MCMC into a single progress bar.

@tpapp
Copy link
Owner

tpapp commented Sep 10, 2020

Thanks, I think this is the best solution for now.

I am merging this, but if that is OK with you I will wait for JuliaNLSolvers/Optim.jl#863 to resolve before tagging a new release. If you need that before, just let me know.

@tpapp tpapp merged commit b9253c1 into tpapp:master Sep 10, 2020
@DilumAluthge DilumAluthge deleted the dpa/progress-meter branch September 10, 2020 09:07
@DilumAluthge
Copy link
Contributor Author

I am merging this, but if that is OK with you I will wait for JuliaNLSolvers/Optim.jl#863 to resolve before tagging a new release. If you need that before, just let me know.

Sounds good to me, there's no time pressure on my part.

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.

Progress report that is a progress bar using ProgressMeter.jl
2 participants