Refactor of Sequential Monte Carlo internals #5274
Conversation
ciguaran
commented
Dec 21, 2021
ciguaran
commented
- This is my first PR ever to Pymc.
- There should not be any breaking changes. This PR modularizes a little bit SMC internals.
- This is an initial refactor, that adds documentation and makes it easier for a new reader to find where the actual SMC sampling happens, vs what checks/stats are generated.
- Maybe some reviewer can answer the question in the comment line 86 from test_smc.
|
@aloctavodia FYI |
|
|
||
| assert_random_state_equal( | ||
| initial_rng_state, np.random.get_state() | ||
| ) # TODO: why this? maybe to verify that nothing was sampled? |
There was a problem hiding this comment.
The PR where this test was added provides some context: #5131
Since it was not obvious why it was there, a comment before the assert explaining the purpose of the check could be helpful.
| @@ -0,0 +1,51 @@ | |||
| import multiprocessing as mp | |||
There was a problem hiding this comment.
I am not sure this needs to be in its own separate file. The original file is pretty concise
There was a problem hiding this comment.
Right I didn't extracted due to size, but because it feels that running parallel vs sequential is kind of independent of SMC, and that it could be reused somewhere else. LMK if its enough reason to be outside or I move it again in.
There was a problem hiding this comment.
If a similar need emerges on another sampler we can refactor it then. Doing it now seems like some premature refactoring.
Also it wouldn't make sense to host it inside the SMC module
|
@ciguaran Thanks for opening the PR. I left some comments above. Since the purpose of this PR is just to clean things out a bit, it would be useful if the commit history is also cleaned up, so that we don't mix changes that have different purposes (like tests doc-strings vs internal refactoring). There is a guide here that might provide some help: https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes |
cool, do you prefer me to squash commits or create a new PR? |
You can try the first, it's a good practice. If it fails miserably you can always open a new PR |
|
Closing to fix commit messages easily, please look at #5281 instead |
