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

added new style of thermostat the logistic thermostat #1026

Closed
wants to merge 4 commits into from
Closed

added new style of thermostat the logistic thermostat #1026

wants to merge 4 commits into from

Conversation

samuelcajahuaringa
Copy link
Collaborator

@samuelcajahuaringa samuelcajahuaringa commented Jul 30, 2018

A new deterministic thermostat style is added in this fix nvt/npt the "logistic thermostat".

Purpose

A new style of the deterministic thermostat is added the logistic thermostat proved ergodic in the most difficult case, with the use of the flag logistic the user can be chosen between the Nosé-Hoover thermostat or the logistic thermostat to perform simulation in the nvt or the npt ensemble.

Author(s)

Samuel Cajahuaringa (Unicamp/Brazil)

Backward Compatibility

This style of the thermostat is added with minor changes in this fix, reusing all functionalities of the fix_nh.cpp, like compute the corresponding conservative energy associated with this dynamic, is possible restart the corresponding extended variables to perform a continuous simulation.

Implementation Notes

The functions logistic_temp_integrate() and losgistic_press_integrate() are the logistic thermostats apply to the particles and barostat, respectively. Implementing in the line 2466 to the line 2593.

TODO list for @akohlmey :

  • rebase code to latest development
  • streamline implementation where possible, replace command line logic to use logistic yes/no
  • augment documentation, also for derived classes that are based on FixNH
  • port changes to KOKKOS and other packages that call the Nose-Hoover chain integrator functions in FixNH.

Post Submission Checklist

Please check the fields below as they are completed_

  • The feature or features in this pull request is complete
  • Suitable new documentation files and/or updates to the existing docs are included
  • One or more example input decks are included
  • The source code follows the LAMMPS formatting guidelines

Further Information, Files, and Links

D. Tapias, D. P. Sanders, and A. Bravetti, Geometric integrator for simulations in the canonical ensemble, J. Chem. Phys. 145, 084113 (2016).
D. Tapias, A. Bravetti, and D. P. Sanders, Ergodicity of One-dimensional Systems Coupled
to the Logistic Thermostat, CMST 23, 11 (2017).

@samuelcajahuaringa samuelcajahuaringa changed the title Add files via upload added new style of thermostat the logistic thermostat Jul 30, 2018
@akohlmey akohlmey requested review from athomps and sjplimp July 31, 2018 07:07
@akohlmey
Copy link
Member

akohlmey commented Aug 2, 2018

@samuelcajahuaringa thanks for your pull request submission. we are currently in the process of preparing a stable release, so we will be looking at your contribution after that.

@samuelcajahuaringa
Copy link
Collaborator Author

thanks, Axel Kohlmeyer and LAMMPS developers. I will take this time to review the code.

Copy link
Contributor

@athomps athomps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern now is the complexity of the code changes that are added to fix_nh.cpp. Anything that can be done to reduce and isolate the number of places where Logistic code is added to the main functions would be helpful in reducing the overall complexity of fix_nh.cpp. I understand that some of this is unavoidable, so I leave it to discretion of @samuelcajahuaringa

@akohlmey
Copy link
Member

@samuelcajahuaringa i am looking at merging this change right now, but it is missing an important pre-requisite: there is no addition to the documentation explaining the new flag. i am also wondering if "logistic" is a good name for this flag. it is a word that has different meanings in different languages, so it it might confuse people.

Add additional deterministic thermostat style in this fix nvt/npt the logistic thermostat, proved ergodic in the most difficult case
@akohlmey
Copy link
Member

akohlmey commented Oct 9, 2019

@athomps i've rebased this change to the latest master, added docs, parameter checks and refactored the code. this looks fairly well encapsulated and it was trivial to port it to KOKKOS. no port to other derived NH classes was needed. i think this is now good enough to be included from a formal point of view. i cannot judge the science, though.

Copy link
Contributor

@athomps athomps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. See comment in fix_nh.cpp
  2. Has this been tested to make sure default behaviors (many cases of nvt/nph/npt with logistic no) is unaffected.
  3. The keyword name logistic is good, there will be no confusion
  4. How well does conserve the conserved quantity versus timestep size?

src/fix_nh.cpp Show resolved Hide resolved
@akohlmey akohlmey added the test_for_regression Enable to trigger regression tests label Oct 10, 2019
@akohlmey
Copy link
Member

  1. See comment in fix_nh.cpp

done

  1. Has this been tested to make sure default behaviors (many cases of nvt/nph/npt with logistic no) is unaffected.

yes, it passes our extended regression test

  1. The keyword name logistic is good, there will be no confusion

ok.

  1. How well does conserve the conserved quantity versus timestep size?

will need to run some tests for that.

@akohlmey
Copy link
Member

@athomps here is some test about energy conservation comparing etotal with fix_modify 1 energy yes: Nose-Hoover default (tchain 3), with plain Nose-Hoover (tchain 1) and Logistic (equivalent to tchain 1). This is for a 500 atom Argon system with a cutoff of 14 \AA and pair_modify shift yes. 10 ns simulation with 1ns equilibration not shown at 10fs or 20fs timestep respectively.

econs

@akohlmey akohlmey removed the test_for_regression Enable to trigger regression tests label Oct 11, 2019
@athomps
Copy link
Contributor

athomps commented Oct 11, 2019

The logistic thermostat shows 10x worse energy conservation that the Nose-Hoover equivalent (tchain=1) on two different metrics: long time energy drift and short time fluctuations. Either this is a known feature of the logistic thermostat (from a brief review of the cited Tapias2016 paper, this is not clear) or something is wrong with the implementation. @samuelcajahuaringa did you do your own testing on this? The short term fluctuations should be substantially smaller for 10fs versus 20x, but they are about the same. This is almost certainly due to some kind of error in the code, probably a missing term in E_conserved.

@akohlmey
Copy link
Member

@samuelcajahuaringa we've discussed the state of this pull request among the LAMMPS developers and have concluded, that it needs more work. There is little value to having improved sampling, when it cannot provide energy conservation to be at least on par with the Nose-Hoover variant with tchain 1. So you either need to demonstrate that energy conservation is better than what our tests indicate, or address the issue in the code, since it seems more likely, that there is something missing or not 100% correct. Since we don't want to spend any more effort keeping this up-to-date, this pull request is now closed and we encourage you to resubmit an updated version. The changes we did to this branch can serve as a guide to how things should be implemented and documented for a successful inclusion. Thanks again for your contribution.

@akohlmey akohlmey closed this Oct 15, 2019
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.

None yet

3 participants