Refactor thermostats: RNG and code duplication #3461
Conversation
The uniform noise function generates 60% less assembly instructions, the gaussian noise function generates 10% less assembly instructions in GCC. In Clang, uniform has the same size, gaussian is 10% smaller.
Reduce code duplication by using the existing thermostat framework.
The method `as_matrix` was not implemented before v1.4.0. Fixes `AttributeError: 'Rotation' object has no attribute 'as_matrix'`.
Codecov Report
@@ Coverage Diff @@
## python #3461 +/- ##
=======================================
- Coverage 86% 86% -1%
=======================================
Files 539 537 -2
Lines 25674 24721 -953
=======================================
- Hits 22282 21335 -947
+ Misses 3392 3386 -6
Continue to review full report at Codecov.
|
| return Utils::Vector3d{uniform(noise[0]), uniform(noise[1]), | ||
| uniform(noise[2])} - | ||
| Utils::Vector3d::broadcast(0.5); | ||
| auto const integers = philox_4_uint64s<salt>(counter, key1, key2); |
There was a problem hiding this comment.
Wouldn't it be sufficient to use a Philox2x64 here?
There was a problem hiding this comment.
Philox2x64 can only take 1 particle id for the key:
src/core/random.hpp:95:18: error: too many initializers for ‘const key_type {aka const r123array1x64}’
const key_type k{id1, id2};
Although the function that generates 1 random uniform value is only called in an NPT function that has no second particle (hence the second key is always 0), we could in the future have a need to generate 1 random value from two particle ids. We would also introduce ambiguity in the function call, since the function generating N=2-4 random values has an optional argument.
There was a problem hiding this comment.
It takes 64 bits as key, e.g. 2 * 32 bit or 2 int.
src/core/thermostat.hpp
Outdated
| /** Is the RNG counter initialized */ | ||
| bool rng_is_initialized() const { return rng_counter != nullptr; } | ||
|
|
||
| private: | ||
| /** RNG counter. */ | ||
| std::unique_ptr<Utils::Counter<uint64_t>> rng_counter; |
There was a problem hiding this comment.
There is no need anymore to store this on the heap right? A boost::optional<uint64_t> would too I think.
|
```
+private:
/** RNG counter. */
std::unique_ptr<Utils::Counter<uint64_t>> rng_counter;
```
There is no need anymore to store this on the heap right? A `boost::optional<uint64_t>` would too I think.
IIRC, there were performance issues with optioals in the context of bonded interactions. Would those apply here or was that related to frequent creation of an optional?
|
|
No, doesn't matter here, it's one static optional per thermostat. For an int an optional is also basically an pair<int, bool>... |
Use boost::optional and C++14 std::enable_if_t. Simplify code.
jngrad
force-pushed
the
thermostats-rng
branch
from
b28be2b to
de56701
Compare
Description of changes:
BaseThermostatthermostat.hpp