Thermalized bond philox

[KonradBreitsprecher]

Fixes the TB part of #2683

• Common philox counter for all thermalized bonds. Decorrelation via the bond partner IDs (not taking into account the bond ID). This was much easier to implement and is valid, as two (different) thermalized bonds on the same particle pair makes no sense.

[jngrad]

• overall LGTM
• There are merge conflicts with 2 of my PRs, one of which is still open (New logic for default parameters in IA classes #3081). I'll take care of resolving the merge conflicts once the review is finished.
• @KaiSzuttor my comments on the RNG seed being valid even if zero/negative are not specific to this PR, they also hold for classes in espressomd.thermostat.pyx.

[jngrad]

The check above only verifies it's an int, so seed=-12 is sent to the core as 18446744073709551604 and after 12 increments it'll be back to 0, then 1, ...

[KonradBreitsprecher]

Should check_type_or_throw_except handle that centrally as this happens also for DPD and Langevin Thermostat?

[jngrad]

A case was made against using is_valid_type to check values (#3080), the same argument can be used against using check_type_or_throw_except. However, creating a seed-dedicated function in utils.pyx that handles both type checking and signedness centrally would probably be fine.

[jngrad]

1. if multiple thermalized bonds are created on different particles, is the global TB seed overwritten each time?
2. the corresponding documentation in inter_bonded.rst is now out of sync. I would suggest we remove the duplicated argument descriptions in inter_bonded.rst and add a link to the Python class, like we do for all other bonded IA classes. This is something I should do because it'll cause a merge conflict with one of my recently merged PRs.

[KonradBreitsprecher]

1. If a seed is provided on creation of the bond, the global counter for all TBs is set to that value. I realise now that this causes trouble in the (very special) case of a second (third..) TB with the same seed that will 'reset' the thermalization of all the TBs.
2. Thanks!

[jngrad]

What do you think of this approach:

    def _set_params_in_es_core(self):
        if thermalized_bond_is_seed_required():
            if self.params["seed"] is None:
                raise ValueError(
                    "A seed has to be given as keyword argument on first activation of the thermalized bond")
            else:
                utils.check_type_or_throw_except(
                    self.params["seed"], 1, int, "seed must be a positive integer")
                thermalized_bond_set_rng_state(self.params["seed"])
        elif self.params["seed"] is not None:
            raise ValueError(
                "A seed has to be given as keyword argument only on first activation of the thermalized bond")

        thermalized_bond_set_params(
            self._bond_id, self._params["temp_com"], self._params["gamma_com"],
            self._params["temp_distance"], self._params["gamma_distance"], self._params["r_cut"])

>>> import espressomd
>>> system = espressomd.system.System(box_l=3*[12])
>>> tb1 = espressomd.interactions.ThermalizedBond(temp_com=300,gamma_com=50,temp_distance=300,gamma_distance=20,r_cut=1)
>>> tb2 = espressomd.interactions.ThermalizedBond(temp_com=300,gamma_com=50,temp_distance=300,gamma_distance=20,r_cut=1,seed=5)
>>> system.bonded_inter.add(tb1) # fails: has no seed
ValueError: A seed has to be given as keyword argument on first activation of the thermalized bond
>>> system.bonded_inter.add(tb2) # ok: has a seed
>>> system.bonded_inter.add(tb2) # fails: overwrites seed
ValueError: A seed has to be given as keyword argument only on first activation of the thermalized bond
>>> system.bonded_inter.add(tb1) # ok: has no seed

[jngrad]

Update on my approach: it's not a good idea. It's problematic when setting TBs in a for loop. Also when setting a TB in multiple test methods of a unittest class, since the order of the tests is based on the alphabetical order of the methods and the presence/absence of fixtures, the only way to reliably set the seed only once is to implement an MPI-safe lock mechanism... We'll just leave the seed logic as it currently is.

[codecov]

Codecov Report

Merging #3070 into python will decrease coverage by <1%.
The diff coverage is 89%.

@@           Coverage Diff           @@
##           python   #3070    +/-   ##
=======================================
- Coverage      83%     83%    -1%     
=======================================
  Files         530     530            
  Lines       26100   25966   -134     
=======================================
- Hits        21915   21770   -145     
- Misses       4185    4196    +11

Impacted Files	Coverage Δ
src/core/random.hpp	58% <ø> (ø)	⬆️
...re/bonded_interactions/bonded_interaction_data.hpp	100% <ø> (ø)	⬆️
src/core/bonded_interactions/thermalized_bond.hpp	100% <100%> (ø)	⬆️
src/core/integrate.cpp	75% <100%> (ø)	⬆️
src/core/bonded_interactions/thermalized_bond.cpp	93% <77%> (-7%)	⬇️
...ore/grid_based_algorithms/lb_particle_coupling.cpp	88% <0%> (-8%)	⬇️
src/core/global.cpp	74% <0%> (-7%)	⬇️
src/core/rotation.cpp	91% <0%> (-7%)	⬇️
...rc/core/electrostatics_magnetostatics/scafacos.cpp	65% <0%> (-6%)	⬇️
src/core/grid_based_algorithms/lb_interface.cpp	72% <0%> (-4%)	⬇️
... and 26 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2948859...99e2a57. Read the comment docs.

[RudolfWeeber]

On Tue, Aug 20, 2019 at 10:25:16AM -0700, Jean-Noël Grad wrote: jngrad commented on this pull request. > @@ -2250,6 +2250,11 @@ class ThermalizedBond(BondedInteraction): distance vector of the particle pair. r_cut: :obj:`float`, optional Specifies maximum distance beyond which the bond is considered broken. + seed : :obj:`int` + Counter value of the philox RNG. Initially, this defines the seed of + the RNG. If prompted, it does not return the initially set counter value + (the seed) but the current state of the RNG. This approach can be problematic when setting TBs in a for loop. Also when setting a TB in multiple test methods of a unittest class, since the order of the tests is based on the alphabetical order of the methods and the presence/absence of fixtures, the only way to reliably set the seed only once is to implement an MPI-safe lock mechanism... We'll just leave the seed logic as it currently is.

Whatever is chosen, here, it needs to be tested that the RNG state is restored when loading form a checkpoint.

[jngrad]

@RudolfWeeber is the test in 1bda0e9 sufficient?

[jngrad]

bors r+

[bors]

Build succeeded

• ICP GitLab CI