Add feedback optimization for PT temperatures #9
Conversation
|
Matt's PT changes look good to me. Also Cole updated some things in the docs regarding the unit system, etc. I think we can accept both, but I'm curious how these two different things ended up in the same PR? |
That's on me! I noticed that there were some issues in the docs from when I merged |
|
After some initial confusion, it wasn't a big deal, I could just look at each commit individually. |
|
OK for me to squash and merge? |
|
I wanted to take a quick look through if that's ok. I can't right now, but I can by the end of the day. |
|
Of course -- handing off to you. |
There was a problem hiding this comment.
I left some suggestions on various changes, but it looks awesome!
In reviewing your code, I noticed some areas where I didn't fully make the LangevinSampler expose the same API as the Metropolis-based samplers which you depend on in some functions. After you make your pass, I'll fix this so that everything should work with both samplers and add a commit to the PR if that's fine with you!
src/ParallelTempering.jl
Outdated
| f = ((N_labeled > 0) ? replica.N_down/N_labeled : 0.0) | ||
|
|
||
| # gather temperatures and replica flow 'f' from all replicas | ||
| gv = MPI.Gather([1.0/replica.sampler.β, f], 0, MPI.COMM_WORLD) |
There was a problem hiding this comment.
Tiny nitpick, but maybe a better variable name than gv?
There was a problem hiding this comment.
I'm also realizing that replica.sampler.β will error with Langevin sampling too, and I've provided no way to get the temperature back out of a LangevinSampler! I can add another commit to this PR adding a new function like get_temp for all samplers that returns their temperature that we can call here instead -- I'll change this and other lines that grab the temperature in my commit.
So that we don't get conflicts, I'll wait until after you've made your pass?
There was a problem hiding this comment.
Awesome, thanks! Just one suggested change making the vector-accepting run_PT! a bit easier since you can pass variable number of keyword arguments through.
I can make those changes to LangevinSampler I alluded to, along with this other run_PT! change (if that seems fine with you), and push another commit onto here.
|
Sounds good. Thanks so much for the suggestions! |
ColeMiles
force-pushed
the
feedback-opt-PT
branch
from
b91251a to
59f30e9
Compare
There was a problem hiding this comment.
Alright, I've added my commit which adds the get_temp function to all of the samplers and changed the PT code to use that function.
Please check that I haven't broken your PT examples in doing this! I don't currently have access to a cluster on which to test the PT code, and I'm having trouble running the scripts through MPI on my personal machine.
Other than testing I haven't broken anything, this seems good to go!
Fix comment typos
No description provided.