Introduce step-setup in samplers - #8351
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8351 +/- ##
==========================================
- Coverage 91.73% 90.12% -1.61%
==========================================
Files 128 128
Lines 20672 20697 +25
==========================================
- Hits 18963 18653 -310
- Misses 1709 2044 +335
🚀 New features to boost your workflow:
|
| tune = get_default_tune_steps(step, tune) | ||
|
|
||
| for method in flatten_steps(step): | ||
| if hasattr(method, "setup"): |
There was a problem hiding this comment.
can't we add it to the baseclass so we don't need to hasattr check it?
There was a problem hiding this comment.
We don't need it, I just added the hasattr so that pm.sample() does not throw.
There was a problem hiding this comment.
I see there's a set_rng... could this be passed into setup? I haven't checked when/where is it called from
There was a problem hiding this comment.
I think set_rng is called per chain. If we move set_rng to setup, then the rng would be shared across chains, and vice versa if we move setup to set_rng then it would be called for each chain repeatedly. That would not be an issue in the current implementation of BART because we can check if setup has already been called. Then the method should be renamed to something like setup_chain
There was a problem hiding this comment.
Yeah setup_chain sounds good enough, and we combine both sources of info
…d tune as well as chain rng
ricardoV94
added
maintenance
major
2 participants
Description
I added a function to
pm.sample(...)that passes the number of draws to the step methods if they have thesetupattribute. With this setup method, we can pre-allocate a vector to storeTreeBatchfor each sampling draw inbartrs.Related Issue
Checklist
Type of change