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

Remove Langevin noise correlation #3355

Merged
merged 7 commits into from
Dec 7, 2019
Merged

Remove Langevin noise correlation #3355

merged 7 commits into from
Dec 7, 2019

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Dec 4, 2019

Fixes #3343

Description of changes:

  • decoupled translational and rotational Langevin RNG seeds
  • add a test for Langevin noise correlation
  • cleanup docs

jngrad and others added 4 commits December 4, 2019 17:51
Remove duplicate Doxygen blocks, updated outdated docs, cleanup
Doxygen syntax, add cross-links.
Remove correlation between translation noise and rotation noise in
the Langevin thermostat.
Co-authored-by: Rudolf Weeber <weeber@icp.uni-stuttgart.de>
@jngrad
Copy link
Member Author

jngrad commented Dec 4, 2019

@RudolfWeeber I can't get rotational-diffusion-aniso.py to pass on my machine without noise correlation, even after restoring the test to its state before your speed-up modifications. The outcome of the test also seems to be very sensitive on the numpy seed. You might want to have a look if CI fails.

@bindgens1
Copy link
Member

I also had problems with the rotation tests in the modifications that @fweik did in #2957
More samples for the histogram didn't help solving the problem. However, I have no idea, if the issues are related to one another or if this is an independent issue.

@RudolfWeeber
Copy link
Contributor

Please use kT <=1.5
@psci2195, do you know why the test doesn't work for higher temperatures?
Is there an assumption about Peclet numbers in the theory?

@jngrad
Copy link
Member Author

jngrad commented Dec 5, 2019

@RudolfWeeber the failing dpd.py test does not fail on my machine, even within a docker container. The test is also extremely sensitive to the seed of the Langevin thermostat.

@fweik
Copy link
Contributor

fweik commented Dec 5, 2019

The failure is due to bad statistics. See #3360, there is no problem. Also this is just thermalized degrees of freedom friction-coupled to other thermalized degrees of freedom, so this should work. This is no different than coupling to a thermalized LB fluid.
I think you can just cherry-pick 05daf01.
I checked multiple seeds, this is all very robust. The test in principle now also passes with 20 loops,
or with a almost tenfold lower error.

@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #3355 into python will increase coverage by <1%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3355    +/-   ##
=======================================
+ Coverage      86%     86%   +<1%     
=======================================
  Files         537     537            
  Lines       25283   25283            
=======================================
+ Hits        21756   21758     +2     
+ Misses       3527    3525     -2
Impacted Files Coverage Δ
src/core/random.hpp 70% <ø> (ø) ⬆️
src/core/thermostat.cpp 97% <ø> (ø) ⬆️
src/core/thermostat.hpp 96% <100%> (ø) ⬆️
src/script_interface/accumulators/TimeSeries.hpp 62% <0%> (+6%) ⬆️

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 6f89e49...f81e7b0. Read the comment docs.

@jngrad
Copy link
Member Author

jngrad commented Dec 6, 2019

@fweik @KaiSzuttor Thanks!
@RudolfWeeber Ready for review

@jngrad
Copy link
Member Author

jngrad commented Dec 7, 2019

bors r=RudolfWeeber

bors bot added a commit that referenced this pull request Dec 7, 2019
3355: Remove Langevin noise correlation r=RudolfWeeber a=jngrad

Fixes #3343

Description of changes:
- decoupled translational and rotational Langevin RNG seeds
- add a test for Langevin noise correlation
- cleanup docs

Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de>
@bors
Copy link
Contributor

bors bot commented Dec 7, 2019

Build succeeded

@bors bors bot merged commit f81e7b0 into espressomd:python Dec 7, 2019
@jngrad jngrad deleted the fix-3343 branch January 18, 2022 12:08
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.

Correlated noise
4 participants