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

core: thermostat: Removed heat_up/cool_down mechanism for Langevin ty… #3341

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Conversation

fweik
Copy link
Contributor

@fweik fweik commented Nov 26, 2019

…pe thermostats

The Langevin-type thermostats (excluding LB), had broken initial forces.
They did still employ the heat_up/cool_down mechanism which is no longer
correct with the philox rng. This is potentially a serious bug, because it makes
the thermostat silently act with a different temperature than set for at least
parts of the time steps.

@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #3341 into python will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3341    +/-   ##
=======================================
+ Coverage      86%     86%   +<1%     
=======================================
  Files         525     538    +13     
  Lines       25005   25312   +307     
=======================================
+ Hits        21507   21784   +277     
- Misses       3498    3528    +30
Impacted Files Coverage Δ
src/core/dpd.cpp 93% <ø> (-7%) ⬇️
src/core/dpd.hpp 100% <ø> (ø) ⬆️
src/core/thermostat.hpp 96% <ø> (ø) ⬆️
src/core/bonded_interactions/thermalized_bond.cpp 81% <ø> (-12%) ⬇️
src/core/bonded_interactions/thermalized_bond.hpp 100% <ø> (ø) ⬆️
src/core/thermostat.cpp 97% <ø> (-1%) ⬇️
src/core/integrate.cpp 80% <ø> (-1%) ⬇️
src/core/virtual_sites/VirtualSitesRelative.cpp 86% <0%> (-3%) ⬇️
... and 22 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 9415194...9c4b4e9. Read the comment docs.

@KaiSzuttor
Copy link
Member

why hast that been caught by any testcase?

@fweik
Copy link
Contributor Author

fweik commented Nov 26, 2019

Because it was not tested.

@KaiSzuttor
Copy link
Member

Shouldn't the DPD test for the velocity distribution fail if the noise magnitude is wrong?

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Nov 27, 2019 via email

@fweik fweik changed the title core: thermostat: Removed head_up/cool_down mechanism for Langevin ty… core: thermostat: Removed heat_up/cool_down mechanism for Langevin ty… Nov 28, 2019
@KaiSzuttor
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Dec 3, 2019
3341: core: thermostat: Removed heat_up/cool_down mechanism for Langevin ty… r=KaiSzuttor a=fweik

…pe thermostats

The Langevin-type thermostats (excluding LB), had broken initial forces.
They did still employ the heat_up/cool_down mechanism which is no longer
correct with the philox rng. This is potentially a serious bug, because it makes
the thermostat silently act with a different temperature than set for at least
parts of the time steps.


Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de>
@bors
Copy link
Contributor

bors bot commented Dec 3, 2019

Build succeeded

@bors bors bot merged commit 9c4b4e9 into espressomd:python Dec 3, 2019
@fweik fweik added this to the Espresso 4.1.2 milestone Dec 3, 2019
@fweik fweik deleted the heat branch August 27, 2020 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants