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

Permanent PLUMED interface #1180

Merged
merged 55 commits into from Nov 8, 2018

Conversation

@gtribello
Copy link
Contributor

gtribello commented Oct 28, 2018

Purpose

The pull request adds a user contributed module that ensures a permanent interface to the plugin for molecular dynamics PLUMED can be called from LAMMPS. The user can thus compile a version of LAMMPS that can use a plumed fix without having to do the patching procedure that is detailed on the PLUMED website.

Author(s)

Pablo Piaggi, Ecole Polytechnique Federale de Lausanne and Universita della Svizzera Italiana, Switzerland
Gareth Tribello, Atomistic Simulation Centre, School of Mathematics and Physics, Queen's University Belfast, Belfast, UK

Backward Compatibility

Nothing has been changed.

Implementation Notes

I implemented the interface with help that was given via email from Axel and Steve. To test the interface I ran the plumed integration tests that are detailed at the bottom of this page: http://plumed.github.io/doc-master/developer-doc/html/_how_to_contribute_to_plumed.html

I have created regression tests that can be used to test whether these things continue to work in the future and put them in a git repository (https://github.com/gtribello/test-lammps-and-plumed). The intention is to use travis.ci to regularly test the interface in future to ensure that the interface keeps working in future.

I was unsure how to add cmake support with this feature and was advised by Axel to submit the pull request without fixing this feature.

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

The PLUMED website is here: https://plumed.github.io
The PLUMED github page is here: https://github.com/plumed/plumed2
The PLUMED documentation can be found here: http://www.plumed.org/documentation

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

@akohlmey akohlmey self-assigned this Oct 28, 2018

Show resolved Hide resolved doc/src/fix_plumed.html Outdated
Show resolved Hide resolved src/.gitignore Outdated
Show resolved Hide resolved src/USER-PLUMED/Install.sh Outdated
Show resolved Hide resolved src/USER-PLUMED/fix_plumed.h Outdated
@junghans

This comment has been minimized.

Copy link
Member

junghans commented Oct 28, 2018

I can help with the CMake, but the PKG_USER-SCAFACOS coe in cmake/CMakeLists.txt should serve as a good starting point.

@gtribello gtribello requested a review from junghans as a code owner Oct 28, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 4, 2018

@PabloPiaggi

This comment has been minimized.

Copy link
Collaborator

PabloPiaggi commented Nov 4, 2018

I think being able to choose an arbitrary compute pressure and compute pe is useful but very dangerous. The computes that are now defined are the only ones that lead to well defined dynamics in simulations in which the energy is biased. I don't think it is a good idea to allow users to perform wrong simulations. If you nonetheless wish to do so, please add a strong warning in the manual.

Furthermore, I had added an error if the user tries to employ tail corrections for LJ since this adds a term to the energy that does not depend on the positions, and thus can screw up the dynamics. Since you are giving a lot of flexibility, perhaps this error should be transformed into a warning.

Lastly, there's the issue that fix plumed forces tallying the energy and virial at every timestep, even when these quantities are not needed. This will slow down simulations that do not bias the energy. The main problem is that Lammps has to know if these quantities are needed in the previous timestep while plumed knows it only in the current timestep. I couldn't find a solution but perhaps there is one.

Thanks Axel and Gareth for the great work!

Pablo

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 5, 2018

various small changes and reformatting
- add memory usage estimator
- test against varying number of atoms
- test against non-consecutive atom tags
- test for 32-bit overflow in number of atoms
- test for 32-bit overflow in timestep
- reduce tail correction error to warning
- more LAMMPS style formatting of the source code
- remove trailing whitespace
- avoid leaking memory from allocated arrays for masses/charges/tags

@akohlmey akohlmey assigned junghans and unassigned akohlmey Nov 5, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 5, 2018

@junghans all yours now for working out cmake support. please note, that linkage has changed for a more stable and consistent static linkage. it should be possible to import the settings from one of the generated .cmake files. Install.py uses the corresponding make file include, which defines some variables.

@junghans

This comment has been minimized.

Copy link
Member

junghans commented Nov 5, 2018

@akohlmey the plumed ".cmake" files aren't available at CMake time, we could use them for the external detection though, but there pkg-config also seems to be the easier choice.

WIth plumed-2.5b I am hitting plumed/plumed2#402.

@GiovanniBussi

This comment has been minimized.

Copy link

GiovanniBussi commented Nov 5, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 5, 2018

@junghans i don't understand what your problem is. since currently the only supported linkage mode is "static" (we could look into others later, if there is demand), the case, where we download plumed leads to a temporary installation only, where it doesn't matter whether LD_LIBRARY_PATH is set or not. all we need is the "static" makefile template, that sets $(PLUMED_LOAD), which is then merged with Makefile.lammps.static to form Makefile.lammps, which is then used to augment the linker command line when building the LAMMPS executable (or shared library). none of those steps ever requires a shared library from the temporary plumed installation and the resulting LAMMPS executable does not depend on any either.

@junghans

This comment has been minimized.

Copy link
Member

junghans commented Nov 5, 2018

@akohlmey sorry I meant to say the .cmake files are NOT available at CMake time, however as it looks to me PLUMED_LOAD is basically just -ldl, which stored in CMAKE_DL_LIBS in CMake and I used that instead.

@junghans junghans assigned akohlmey and unassigned junghans Nov 5, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 5, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 5, 2018

@junghans

This comment has been minimized.

Copy link
Member

junghans commented Nov 5, 2018

@akohlmey ok I see what you are saying, however I am confused now why CMake even links against libplumedWrapper.a without GSL.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 5, 2018

@junghans

This comment has been minimized.

Copy link
Member

junghans commented Nov 5, 2018

@akohlmey, ok, I see, for external plumed we can just do an include(Plumed.cmake.static), however for the internal download and build, I will have to think about it for a bit as Plumed.cmake.static isn't available before building and hence at CMake time.

@junghans

This comment has been minimized.

Copy link
Member

junghans commented Nov 5, 2018

@akohlmey that should do it for now.

@@ -540,12 +541,14 @@ if(PKG_USER-PLUMED)
ExternalProject_get_property(plumed_build INSTALL_DIR)
set(PLUMED_INSTALL_DIR ${INSTALL_DIR})
list(APPEND LAMMPS_DEPS plumed_build)
list(APPEND LAMMPS_LINK_LIBS ${PLUMED_INSTALL_DIR}/lib/libplumedWrapper.a ${CMAKE_DL_LIBS})
list(APPEND LAMMPS_LINK_LIBS ${PLUMED_INSTALL_DIR}/lib/plumed/obj/kernel.o

This comment has been minimized.

@akohlmey

akohlmey Nov 5, 2018

Member

this is different for plumed 2.5

This comment has been minimized.

@junghans

junghans Nov 5, 2018

Member

In CMake, the version is hard-coded, so we can fix it, when we bump it.

As said above due to plumed/plumed2#402 I cannot even build plumed-2.5 at this point in time.

This comment has been minimized.

@GiovanniBussi

GiovanniBussi Nov 5, 2018

@junghans you can just merge the fix or configure plumed without python (--disable-python).

This comment has been minimized.

@junghans

junghans Nov 5, 2018

Member

Let's just bump the version to 2.5 in CMake, when you guys released it.

This comment has been minimized.

@GiovanniBussi

GiovanniBussi Nov 5, 2018

@junghans following our usual schedule v2.5 is planned for (approx) December.

This comment has been minimized.

@junghans

junghans Nov 5, 2018

Member

If you guys prefer the beta version of 2.5 instead of 2.4.3 to be part of CMake we could bump it now.

@akohlmey akohlmey requested review from sjplimp and stanmoore1 Nov 5, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Nov 5, 2018

@sjplimp

sjplimp approved these changes Nov 8, 2018

Copy link
Contributor

sjplimp left a comment

all looks fine to me

@akohlmey akohlmey merged commit a6e9b99 into lammps:master Nov 8, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment