Added Kipton's spherical cap update and include Gaussian spin update. #3
Conversation
src/WangLandau/WangLandau.jl
Outdated
|
|
||
| E_next = E_curr + local_energy_change(WLS.system, pos, new_spin) / ps | ||
|
|
||
| Δln_g = ln_g_tmp[E_curr] - ln_g_tmp[E_next] | ||
|
|
||
| if (Δln_g >= 0) || ( rand(WLS.rng) <= exp(Δln_g) ) | ||
| if (Δln_g >= 0) || ( rand() <= exp(Δln_g) ) |
There was a problem hiding this comment.
Why did this lose WLS.rng?
There was a problem hiding this comment.
Oops. It should use WLS.rng.
src/WangLandau/WangLandau.jl
Outdated
| @@ -247,7 +292,7 @@ function run!(WLS::WangLandau) | |||
| Δln_g = WLS.ln_g[E_curr] - WLS.ln_g[E_next] | |||
|
|
|||
| # accept move | |||
| if (Δln_g >= 0) || ( rand(WLS.rng) <= exp(Δln_g) ) | |||
| if (Δln_g >= 0) || ( rand() <= exp(Δln_g) ) | |||
There was a problem hiding this comment.
Same here -- is the removal of WLS.rng intentional?
There was a problem hiding this comment.
Also should use WLS.rng
src/WangLandau/WangLandau.jl
Outdated
| -r y 0 | ||
| z*y/r z x/r ] | ||
| S′ = Vec3(x′, z′, y′) | ||
| return R*S′ |
There was a problem hiding this comment.
Really minor style comment, but maybe move the return R*S' outside of the if/else?
There was a problem hiding this comment.
It is interesting to me that this (Cole's suggested modification) indeed works even though the local variables R and S' are not defined before the if/else/end block.
src/WangLandau/WangLandau.jl
Outdated
|
|
||
| ℓ = sqrt(S[1]^2 + S[2]^2) | ||
| function spherical_cap_update(S::Vec3, cos_max_angle::Float64, ξ1::Float64, ξ2::Float64)::Vec3 |
There was a problem hiding this comment.
This function has been added, but just verifying that it seems like the WangLandau isn't actually using it? (Using the gaussian_spin_update instead?)
There was a problem hiding this comment.
Correct, I guess which one to pick is sort of a wash according to benchmarks.
| -ℓ*x + S[3]*z | ||
| ) | ||
| """ | ||
| Generate a random unit spin that is normally distributed about the direction |
There was a problem hiding this comment.
I'd maybe consider changing the name of this function and/or the docstring. The function isn't actually generating any randomness, right? It's really just doing a vector addition and re-normalization.
There was a problem hiding this comment.
Maybe we should be passing the rng to the "sample" functions instead of the randomly sampled numbers. (For background, I originally organized the code this way because it made it easier to test things, but now that we know it works, I think it's cleaner to pass the rng).
There was a problem hiding this comment.
Good point. I had it calling randn() originally (hence the missing WLS.rng in the other comments).
Just saw Kipton's comment about passing the rng - I can do that
|
Gentle ping. I think this can be merged with some minor changes. I guess it makes sense to pass the rng to |
|
Sorry for the delay. I should be able to make these changes in little bit. |
|
I made the changes here, so not sure it's ready to merge now. I passed a Random.AbstractRNG type as a parameter to the update functions. |
No description provided.