-
Notifications
You must be signed in to change notification settings - Fork 19
Progress bars when sampling multiple chains #168
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
base: main
Are you sure you want to change the base?
Conversation
6c3d929
to
8eec9e9
Compare
AbstractMCMC.jl documentation for PR #168 is available at: |
8a9d376
to
1195503
Compare
A gentle reminder @nsiccha |
Will try to get to this later today :) |
Will only get to it tomorrow. |
function CreateNewProgressBar(name::AbstractString) | ||
return new{typeof(name)}(name, UUIDs.uuid4()) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the below functions have a trailing !
? They aren't mutating the parameters, but some global state, right?
(Doesn't really matter, we just as well leave the names as is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, they are mutating (although the macro hides the worst of it). Will change.
docs/src/api.md
Outdated
|
||
For multiple-chain sampling using `MCMCThreads`, there are several, more detailed, options: | ||
|
||
- `:perchain`: create one progress bar per chain being sampled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:perchain
also creates the overall progress bar, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, although in the case of :perchain
the extra progress bar tracks chains rather than samples. Thanks, have added a line to clarify.
docs/src/api.md
Outdated
## Progress logging | ||
|
||
The default value for the `progress` keyword argument is `AbstractMCMC.PROGRESS[]`, which is always set to `true` unless modified with `AbstractMCMC.setprogress!`. | ||
For example, `setprogress!(false)` will disable all progress logging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording here made me assume that I'd also be able to do setprogress!(:perchain)
, but AFAICT currently this is not possible? Should this be allowed? Maybe not, as the added benefit would be quite small?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of tricky. If you do setprogress!(:perchain)
, the multi-chain methods would understand it, but the single-chain method would have no idea what to do with it. Hence why there are two different settings -- one which takes a boolean, and a second one which controls multi-chain output. You are right that the wording is not that great, I'll make that clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall to me, but I'll have another look (soonish).
@nsiccha Apologies for pinging again. Did you still want to take more looks at it, or was that done already as part of your review above? 😄 |
@penelopeysm no worries. I had been planning to actually run/test the code at least once on my machine, which I haven't gotten around to yet 🙈 |
Okay, I'll wait for you :) |
After me testing the code out locally and us talking on Slack, we've agreed to
I think that was mainly it, wasn't it, @penelopeysm? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have another look after the changes we've discussed.
|
||
For single-chain sampling (i.e., `sample([rng,] model, sampler, N)`), as well as multiple-chain sampling with `MCMCSerial`, the `progress` keyword argument should be a `Bool`. | ||
|
||
For multiple-chain sampling using `MCMCThreads`, there are several, more detailed, options: | ||
|
||
- `:perchain`: create one progress bar per chain being sampled | ||
- `:perchain`: create one progress bar per chain being sampled, plus one progress bar tracking the number of chains | ||
- `:overall`: create one progress bar for the overall sampling process, which tracks the percentage of samples that have been sampled across all chains | ||
- `:none`: do not create any progress bar | ||
- `true` (the default): use `perchain` for 10 or fewer chains, and `overall` for more than 10 chains | ||
- `false`: same as `none`, i.e. no progress bar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be "same as :none
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and also "use :perchain
for 10 or fewer chains, and :overall
" 😅
Summary
This PR introduces more informative progress bars with
MCMCThreads
which can be configured by the user (see options below).In today's meeting it was generally agreed that having one progress bar per chain was more informative / useful, and that an upper bound could be placed to disable that by default if there were more than
MAX_CHAINS_PROGRESS
chains.This PR sets
MAX_CHAINS_PROGRESS = 10
. The user can override the default on a case-by-case basis using theSymbol
versions ofprogress
, or can globally set the threshold usingsetmaxchainsprogress!(N)
.For
MCMCSerial
no change is made (it already had one progress bar per chain).For
MCMCDistributed
,progress = :overall
is implemented (progress = true
is a synonym for that) butprogress = :perchain
isn't, not for any good technical reason, but because it was just too much of a faff. I think that in principle it's possible to implement it similarly to how it's done for MCMCThreads (i.e., generate one UUID per chain), but I think each chain has to communicate back to the parent worker using aRemoteChannel
, and there needs to be a way of disentangling the inputs from different chains (so you either need NRemoteChannel
s or the channel needs to take a chain ID). I suppose if somebody really, really wants it I could do it, but given that:overall
already works I am hoping nobody will complain.In a technical sense this is accomplished by passing richer objects in the
progress
keyword argument (seesrc/logging.jl
). This does mean that people should not overrideprogress
in their own implementations (I have added a warning in the docs) but it is IMO better than using callbacks.The videos below demonstrate the behaviour for
MCMCThreads
.Closes #82
Closes TuringLang/Turing.jl#2264
Setup
progress=:perchain
perchain.mov
progress=:overall
overall.mov
progress=true
true.mov
progress=false
orprogress=:none
false.mov
Bonus:
:perchain
and:overall
on Pluto (it's pretty)Screen.Recording.2025-06-30.at.23.51.20.mov
Bonus:
:perchain
in iJulia (in VSCode; it's not pretty)Screen.Recording.2025-06-30.at.23.59.19.mov
Bonus:
:overall
in iJulia (works fine)Screen.Recording.2025-06-30.at.23.59.46.mov