Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thermostats refactoring #4845

Merged
merged 5 commits into from
Jan 19, 2024
Merged

Thermostats refactoring #4845

merged 5 commits into from
Jan 19, 2024

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Dec 21, 2023

Description of changes:

  • encapsulate thermostats and remove their Cython interface
  • remove 20 MPI callbacks
  • remove thermostat and thermalized/rigid bond global variables
  • remove a significant amount of historical Cython code that relied on deprecated Cython features
  • improve testing of the thermostats interface
  • API changes:
    • thermalized bond parameter seed was moved to system.thermostat.set_thermalized_bond()
      • this change better reflects the fact there is only one global seed for all thermalized bonds
      • until now this global seed was overwritten by any newly created thermalized bond, whether it was added to the system or not
    • LB thermostat seed parameter now gets written to the RNG seed (silent change)

jngrad added 2 commits January 8, 2024 18:54
Fix outdated instructions in chapter on bonded interactions.
Remove automatic linking of words that collide with class names
and namespaces due to the large number of false positives.
API changes: thermalized bond random number generator seed moved to
the thermostat class to avoid accidentally modifying the global seed,
LB thermostat seed parameter is now interpreted as the RNG seed
instead of the RNG Philox counter.
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Several Cython features have been deprecated in Cython 3.0 and will
generate errors in future Cython releases. This change removes .pxi
include files, all `IF` conditional statements, and cpdef functions.
Almost all Cython 3.0 deprecation warnings were addressed, a silent
API change from Python 3.11 was mitigated, and some of the custom
Cython code for type checking was replaced by equivalent C++ code.
@jngrad
Copy link
Member Author

jngrad commented Jan 10, 2024

Regarding the API change for thermalized bonds: the thermalized bond thermostat had always used a single, global RNG state that would be silently overwritten by each new thermalized bond. The new API no longer gives the false impression that each bond maintains its own RNG. The thermostat infrastructure is likely going to change in the near future anyway.

We should really consider rewriting the DPD thermostat to no longer get incremented by external components like constraints. This would allow us to have a single shared RNG counter from which all thermostats could extract their random numbers (the RNG salt decorrelates them). Then the DPD non-bonded potential and thermalized bonds would behave like any other interaction, since they would just fetch the RNG values from the system thermostat. This would also make the thermostat script interface a lot more maintainable. Right now it's quite convoluted, because the specificities of each thermostat prevent me from applying the SOLID principles. In particular, when turning a thermostat on, the arguments are dispatched to the thermostat container (kT) and to the actual thermostat (gamma, seed, etc.), which means that throwing a C++ exception requires rolling back the state of two script interface objects (both of which interact with the system class through distinct event code paths during that rollback) to gracefully restore the system into a valid state. I tried to refactor things further, but it proved to be a can of worm, and I could not find a solution that would both preserve the current API and play nice with the checkpointing mechanism.

In any case, I think this refactoring is a big improvement over the original Cython code, and from my side can be reviewed and merged as is. We can then consult with our users during the next software meeting how we want to proceed with further refactoring of the thermostats, and decide on which API changes would be acceptable. I would prefer not to delay the merge decision for this PR, because it simplifies the LB particle coupling code quite a bit and removes highly coupled global variables, and I would like to leverage that in my ongoing GPU LB work resp. ongoing globals removal work.

@jngrad
Copy link
Member Author

jngrad commented Jan 12, 2024

There might actually be a bug in the LB particle coupling RNG. I have produced a MWE in #4848.

src/python/espressomd/script_interface.pyx Show resolved Hide resolved
testsuite/python/particle.py Show resolved Hide resolved
testsuite/scripts/tutorials/test_polymers.py Show resolved Hide resolved
doc/tutorials/polymers/polymers.ipynb Show resolved Hide resolved
src/script_interface/thermostat/thermostat.hpp Outdated Show resolved Hide resolved
src/core/lb/particle_coupling.hpp Outdated Show resolved Hide resolved
@jngrad jngrad added the BugFix label Jan 19, 2024
@jngrad jngrad added this to the ESPResSo 4.3.0 milestone Jan 19, 2024
reinaual
reinaual previously approved these changes Jan 19, 2024
@jngrad jngrad added the automerge Merge with kodiak label Jan 19, 2024
@kodiakhq kodiakhq bot merged commit da47129 into espressomd:python Jan 19, 2024
10 checks passed
@jngrad jngrad deleted the thermostats branch January 19, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants