Fix issue #377 Make Multiparticle reversible #412
Conversation
…ultiparticle Fix formatting in AlphaNum.h
There was a problem hiding this comment.
Introduced a force_inRange array which is used to indicate whether the translation used the force-bias or was random xyz, so the next move can use this information to reverse the move if it so desires. Also, introduced a key variable to r123 calls. Modified the rotation method.
|
Thanks for adding a description of the changes. I will test some proposed changes and resubmit the patch for approval. |
| double accept = MPCoeff * uBoltz; | ||
| double pr = prng(); | ||
| double pr = r123wrapper.GetRandomNumber(newCOMs.Count()); |
There was a problem hiding this comment.
Is there any reason we dont want to use Mersenne Twister (prng()) here? Since newCOMs.Count() returns the number of molecule (not atoms), we are basically reusing same random number twice.
There was a problem hiding this comment.
The molecule indices are numbered 0 - newCOMs.Count() - 1, so this is really a different one from what was used before. Unless I missed something. I was testing some things when I made this change. I can change it back, but I figured in the long run we want o switch to the same RNG for everything, so...
There was a problem hiding this comment.
Looks good. I thought we draw random number for every atom and not molecule. I was wrong though. Up to you if you want to use prng() or random123
src/moves/MultiParticle.h
Outdated
| #ifdef _OPENMP | ||
| //Even though we call different random123 functions, they all change c[0] | ||
| //and use the same rng() function, so they all need to be in the same | ||
| //critical section (not different names). | ||
| #pragma omp critical | ||
| #endif |
There was a problem hiding this comment.
As discussed, in later version we can implement the random123 CPU version, similar to GPU version to avoid using omp critical overhead.
There was a problem hiding this comment.
Thanks. Yes, I will test and verify that change before merging the patch. I'll push an update if I can get it working without a critical section.
| @@ -525,7 +527,7 @@ inline void MultiParticleBrownian::RotateForceBiased(uint molIndex) | |||
|
|
|||
| matrix = RotationMatrix::FromAxisAngle(rotLen, rot.Normalize()); | |||
|
|
|||
| XYZ center = newCOMs.Get(molIndex); | |||
There was a problem hiding this comment.
I cannot make a comment in Line 464, but similar to Multiparticle, we need to use omp critical for XYZ MultiParticleBrownian::CalcRandomTransform(XYZ const &lb, double const max, uint molIndex) function.
There was a problem hiding this comment.
Yes. I will do that if I can't find a work-around to needing the critical section with the MultiParticle move.
|
I have addressed Mohammad's comment about the omp critical section by revising the Random123Wrapper code to use local variables. Also refactored Random123Wrapper to make it easier to maintain and keep the function calls consistent. Should be ready to merge now. |
No description provided.