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

Fast-forward Langevin functionality included #1137

Merged
merged 7 commits into from Oct 2, 2018

Conversation

@dilkins
Copy link
Contributor

dilkins commented Sep 28, 2018

Purpose

Implementation of the Fast-Forward Langevin (FFL) thermostat, as described in J. Chem. Phys. 148, 184109 (2018).

Author(s)

Lionel Constantin, David M. Wilkins, Michele Ceriotti, EPFL Lausanne

Backward Compatibility

N/A

Implementation Notes

The code for FFL dynamics was based on fix_gle, the code for Generalized Langevin Equation dynamics. Correctness of the algorithm was verified by comparison with the results in Fig. 4 of the paper. No other LAMMPS features are affected.

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

Link to Fast-Forward Langevin paper

@akohlmey akohlmey added this to the Stable Release Fall 2018 milestone Sep 28, 2018

@akohlmey akohlmey requested review from athomps and sjplimp Sep 28, 2018

@akohlmey akohlmey self-assigned this Sep 28, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 28, 2018

@sjplimp since we were discussing renaming of styles in the other pull request, seeing this PR reminds me, that we now have multiple langevin thermostat variants (and with this one more), and perhaps we should consider renaming them to langevin/XXX to make their heritage more visible. OTOH, the naming style here (initials of the name) is consistent with several styles (not just langevin thermostats). Do you have a preference?

------------------------------------------------------------------------- */

#include <mpi.h>
#include <math.h>

This comment has been minimized.

@akohlmey

akohlmey Sep 28, 2018

Member

please use c++ style system headers here: instead of <math.h>, instead of <string.h> and instead of <stdlib.h>.

*
* */

/* ---------------------------------------------------------------------- */

This comment has been minimized.

@akohlmey

akohlmey Sep 28, 2018

Member

For improved consistency with other sources in LAMMPS, we would appreciate, if this could be reformatted to follow the LAMMPS coding style a bit closer. The main differences i currently see is, that usually we use 2 character indentation and put the opening brace of if/for/while statements on the same line, not the next. The main benefit is that diffs become more readable, in case we need to make changes in the future and then the automatic re-indentation would hide the real changes. This is a hard requirement only for files in standard packages, which this is not. So you don't need to do it, if it is too much of a bother.

This comment has been minimized.

@ceriottm

ceriottm Oct 1, 2018

Collaborator

Will do it, makes sense since it's a small package anyway


// Flip type used, uses rescale if no flip is given
if (narg == 8) strcpy(flip_type,arg[7]);
else strcpy(flip_type,"rescale");

This comment has been minimized.

@akohlmey

akohlmey Sep 28, 2018

Member

Please use strncpy() here or make flip_type dynamically allocated, so that flip_type does not overflow and cause a crash on incorrect input.

This comment has been minimized.

@akohlmey

akohlmey Sep 28, 2018

Member

... or use an enumerator as it is done in many similar cases in LAMMPS.

@athomps
Copy link
Contributor

athomps left a comment

The only question I have is why does this need to be a new fix instead of just an option on fix gle? Both fixes are developed in the Ceriotti group and share a lot of functionality. If it is not a lot of work to combine them now, it will save time in the long run.

@ceriottm

This comment has been minimized.

Copy link
Collaborator

ceriottm commented Oct 1, 2018

@athomps I agree it might be nice to "merge" with GLE. main reasons why we did not, and I'm reluctant to do that unless it's a showstopper, are that (1) we haven't really tested how this idea would generalize to the GLE framework (i.e. do you "flip" also the additional momenta or not?) and (2) this simple version should be easily combined with rigid-body integrators, whereas the GLE with rigid bodies is a can of worms that we tried to open a couple of times but ran away in terror.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 1, 2018

@ceriottm @athomps i don't consider merging/consolidating langevin thermostats a showstopper issue. if we would want to do that, i would expand the scope and do this in a more general way, since there are more langevin based thermostats, and they come in different flavors (e.g. with/without time integration). from what i know, there may be more langevin features added in the near future, and i believe it would be better to think about consolidation after that. also, i would have to say, that thermostats in general have been causing very low maintenance effort, if they are well tested at submission time. so the penalty for not merging is not so large as it might seem.

in short, i would suggest we move forward with doing the few small formal adjustments request and then merge this feature afterwards, and then perhaps file a TODO item in the issue tracker to remind us, to make an effort to generalize langevin thermostats across the board (at which point, we can possibly also rethink the naming conventions for those).

@ceriottm

This comment has been minimized.

Copy link
Collaborator

ceriottm commented Oct 1, 2018

@akohlmey OK so I'll push @dilkins and Lionel to go through the style changes quickly.
actually I'm at a conference with Ben Leimkuhler and I'll discuss with him about rigid bodies with GLE, we might try and implement that in LAMMPS, and that would be a good opportunity for consolidation

@athomps

This comment has been minimized.

Copy link
Contributor

athomps commented Oct 1, 2018

Thanks for the explanation. I am okay with keeping GLE and FFL separate.

@dilkins

This comment has been minimized.

Copy link
Contributor

dilkins commented Oct 2, 2018

Thanks all for your comments: I've taken them on board in the two new commits.

@akohlmey
Copy link
Member

akohlmey left a comment

@dilkins it would help readability of the code further, if you would be adding at the top something like this:

enum { NO_FLIP, FLIP_RESCALE, FLIP_HARD, FLIP_SOFT};

and then use the resulting symbolic constants instead of 0, 1, 2, 3, respectively. see for example, how the pcouple keyword is handled in fix_nh.cpp

@dilkins

This comment has been minimized.

Copy link
Contributor

dilkins commented Oct 2, 2018

Apologies, yes, I've included this in d5e71e7

@akohlmey
Copy link
Member

akohlmey left a comment

@dilkins thanks for all the updates

@akohlmey akohlmey merged commit dba8f9c into lammps:master Oct 2, 2018

5 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/kokkos_omp head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details

@dilkins dilkins deleted the dilkins:fast-forward-langevin branch Oct 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment