Conversation
williambdean
force-pushed
the
expose-callback
branch
3 times, most recently
from
46203f6 to
ce92b91
Compare
Extend ChainProgress with optional latest_sample field instead of adding separate SampleCallback. Consolidates callback API while maintaining access to per-sample data.
williambdean
force-pushed
the
expose-callback
branch
from
ce92b91 to
51fd3cb
Compare
| pub runtime: Duration, | ||
| pub divergent_draws: Vec<usize>, | ||
| /// The most recent sample from this chain (if available) | ||
| pub latest_sample: Option<SampleData>, |
There was a problem hiding this comment.
I like the idea of having a separate struct for the draw specific entries, but we should then also move the existing ones into the struct. I don't think we have to worry about backward compatibility here too much, and nuts-rs is still pre-1.0. (But maybe we should change that soonish...)
And do we really need to store the position? I can't really think of anything useful to do with it, given that it's just the unconstrained flattened parameters without any labels. Do you have a specific use-case in mind?
There was a problem hiding this comment.
Removed the position and renamed to be closed to PyMC sampler names.
There was a problem hiding this comment.
Thanks. Can you also put the existing last sample stats (tuning, step_size, latest_num_steps) into the latest_sample struct?
| chains.iter().map(|chain| chain.progress()).collect_vec(); | ||
| responses_tx.send(SamplerResponse::Progress(progress.into())).map_err(|e| { | ||
| anyhow::anyhow!( | ||
| responses_tx |
There was a problem hiding this comment.
Looks like some unrelated formatting changes slipped in?
There was a problem hiding this comment.
I had a bit of a hard time getting the env setup locally. But did run cargo fmt
There was a problem hiding this comment.
Can you just take those changes out of the PR?
|
Coverage is failing for some reason. .. Maybe I should have forked... Logs: https://github.com/pymc-devs/nuts-rs/actions/runs/23067147874 |
| progress | ||
| .lock() | ||
| .expect("Poisoned mutex") | ||
| .update(&info, now.elapsed()); |
There was a problem hiding this comment.
I think we can just put the extra stats you want to expose to the info struct? That way we don't have to call stats.get_all (which allocates a bunch) and also don't lock the progress twice.
2 participants
Getting ball rolling for pymc-devs/nutpie#166
This creates a new constructor
new_with_sample_callbackin order to keep backwardscompatibility.