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

Contribution: package USER-HDNNP for high-dimensional neural network potentials #2626

Merged
merged 55 commits into from
May 25, 2021

Conversation

singraber
Copy link
Collaborator

@singraber singraber commented Feb 25, 2021

Summary

I would like to propose the addition of a new user package "USER-NNP" which requires linkage to the external n2p2 library. The package contains a new pair style "nnp" which allows to use high-dimensional neural network potentials (Behler and Parrinello, Phys. Rev. Lett. 2007, 98 (14), 146401). These machine learning potentials require training before they can be used in production MD runs. Training and related tasks can be performed with tools provided by the n2p2 package. However, n2p2 does not come with its own MD engine (why reinvent the wheel?) and instead relies on an interface with LAMMPS (Singraber, Behler and Dellago, J. Chem. Theory Comput. 2019, 15 (3), 1827-1840) . This has been shipped with n2p2 for quite a while now and I think it would be beneficial for users if it was finally integrated in LAMMPS directly. The n2p2 package is kept compatible with Jörg Behler's original HDNNP software RuNNer regarding its predictive capabilities, i.e. a neural network potential trained with RuNNer can also use the same LAMMPS/n2p2 interface which further increases the potential user base. However, currently only short-range HDNNPs (the most common type used) are supported, but an extension to include electrostatics is work in progress.

I have now integrated the USER-NNP package which was shipped previously with n2p2 directly in LAMMPS and tried to conform to coding standards and include all the required documentation. I hope I did not miss any major problems, however, there are a few open points (see below) for which I would like to ask for your opinion. Overall, I am looking forward to hear your suggestions and whether you think my proposal would fit into the LAMMPS infrastructure.

Thank you for your efforts!

Related Issue(s)

None

Author(s)

Andreas Singraber (andreas.singraber@gmx.at)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

Not aware of any problems.

Implementation Notes

Compiling LAMMPS with the USER-NNP package requires also to build (parts of) n2p2 in advance. This is shortly described in the corresponding "Build Extra" section (3.7.23) of the manual. More detailed information can be found in the lib/nnp/README file. After compiling n2p2 the build process in LAMMPS should be straightforward, following the usual procedure to include user packages. There is one extra flag -D N2P2_DIR which must be set in case n2p2 is not located in lib/nnp/n2p2. To make a serial LAMMPS build also n2p2 must be compiled with MPI disabled, see lib/nnp/README how to achieve that.

IMPORTANT: For now, please ignore the remarks about using n2p2 version v2.2.0 and check out the master branch instead.

No other LAMMPS features are affected by this user package.

Post Submission Checklist

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • Suitable tests have been added to the unittest tree.
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

As mentioned above I want to address a few issues which you may also consider:

  • (1) Name of the package/pair style

    Using NNP/nnp as package/pair style name sounds very general. If you are not happy with that this could be switched to HDNNP/hdnnp or some other descriptive abbreviation (considering that there is a similar project on the way: Add new Package USER-RANN with pair style rann for using a neural network to compute energies and forces #2570).

  • (2) CMake files

    As far as I could test both the CMake and the traditional build method work (also with -DLAMMPS_BIGBIG). However, I have never used CMake before, so there may be some weird/incorrect ways I handled the library search in cmake/Modules/FindN2P2.cmake and cmake/Modules/Packages/USER-NNP.cmake.

  • (3) Older LAMMPS-style code parts

    Some parts in pair_nnp.cpp were written before the Tokenizer class existed and hence look a bit outdated (see PairNNP::settings()). The code works as it is but if you prefer I can rewrite these parts.

  • (4) Element/Type mapping

    At some point I introduced the emap keyword which allows the user to manually assign n2p2 element strings to LAMMPS atom types. This is important because in n2p2 elements are always sorted according to atomic number while in LAMMPS an input configuration could define atoms in a different way. So I don't support any other pair_coeff call besides pair_coeff * * max_cutoff and instead use the user's emap input to sort out atom types I don't need (e.g. in a pair_style hybrid situation). I wonder if the setflag variable which is used in many pair styles is somehow related to this mapping problem? Would you prefer a solution which makes use of the setflag mechanism?

  • (5) Pimpl idiom

    When compiling pair_nnp.cpp the compiler needs to "see" a lot of n2p2 headers down to very basic ones. This is not a problem because they are automatically collected in n2p2's include directory when compiling libnnpif. However, I think the "Pimpl idiom" would be a more clever solution here. Would you prefer/strictly require to implement the interface in this way?

Copy link
Member

@akohlmey akohlmey left a comment

Choose a reason for hiding this comment

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

Thanks for you submission. I have not tried it out yet, but for the most part it looks pretty clean. I noticed a few cosmetic things and added comments in the sources. Will respond to your comments and questions in a separate message.

src/USER-NNP/pair_nnp.cpp Outdated Show resolved Hide resolved
src/USER-NNP/pair_nnp.cpp Outdated Show resolved Hide resolved
src/USER-NNP/pair_nnp.cpp Outdated Show resolved Hide resolved
src/USER-NNP/pair_nnp.cpp Outdated Show resolved Hide resolved
src/USER-NNP/pair_nnp.cpp Outdated Show resolved Hide resolved
src/USER-NNP/pair_nnp.cpp Outdated Show resolved Hide resolved
@akohlmey akohlmey added this to the Stable Release Spring 2021 milestone Feb 26, 2021
@akohlmey akohlmey self-assigned this Feb 26, 2021
@akohlmey
Copy link
Member

As mentioned above I want to address a few issues which you may also consider:

(1) Name of the package/pair style
Using NNP/nnp as package/pair style name sounds very general. If you are not happy with that this could be switched to HDNNP/hdnnp or some other descriptive abbreviation (considering that there is a similar project on the way: #2570).

Switching to USER-HDNNP and pair style hdnnp would be preferred. Due to the large number of research groups investing into this style of potentials there are several of those in development and a few submitted (and one temporarily withdrawn). So to make names less ambiguous is helpful. It should also help to make your contribution more recognizable!

(2) CMake files
As far as I could test both the CMake and the traditional build method work (also with -DLAMMPS_BIGBIG). However, I have never used CMake before, so there may be some weird/incorrect ways I handled the library search in cmake/Modules/FindN2P2.cmake and cmake/Modules/Packages/USER-NNP.cmake.

This is something that @junghans and @rbberger can check out. They are usually pretty good at spotting problems.
One of the biggest concerns is usually maintaining compatibility with the range of Linux "flavors" that we want to be compatible to. And that currently sets the minimum CMake version to 3.10.

(3) Older LAMMPS-style code parts
Some parts in pair_nnp.cpp were written before the Tokenizer class existed and hence look a bit outdated (see PairNNP::settings()). The code works as it is but if you prefer I can rewrite these parts.

This is not an urgent issue unless rewrites are required anyway. I can look at that later after the more general issues have been addressed and I have done some experiments and manual checks.

(4) Element/Type mapping
At some point I introduced the emap keyword which allows the user to manually assign n2p2 element strings to LAMMPS atom types. This is important because in n2p2 elements are always sorted according to atomic number while in LAMMPS an input configuration could define atoms in a different way. So I don't support any other pair_coeff call besides pair_coeff * * max_cutoff and instead use the user's emap input to sort out atom types I don't need (e.g. in a pair_style hybrid situation). I wonder if the setflag variable which is used in many pair styles is somehow related to this mapping problem? Would you prefer a solution which makes use of the setflag mechanism?

See my comment embedded in the code. I am mostly thinking of how familiar LAMMPS users with specifying the element/label mapping on the pair_coeff * * line, while the global cutoff would be specified with the pair_style command.

(5) Pimpl idiom
When compiling pair_nnp.cpp the compiler needs to "see" a lot of n2p2 headers down to very basic ones. This is not a problem because they are automatically collected in n2p2's include directory when compiling libnnpif. However, I think the "Pimpl idiom" would be a more clever solution here. Would you prefer/strictly require to implement the interface in this way?

What you currently have is already sufficiently pimpl-ified for our needs. There is little concern about which headers are included in the pair*.cpp files, especially when the include statement follows all LAMMPS headers. The problems arise with including headers in the pair*.h file (or people putting a "using namespace XXX;" there). That can lead to clashes with the way we currently built the style name to creating a class instance map.

@singraber singraber marked this pull request as draft March 8, 2021 09:02
@akohlmey
Copy link
Member

@singraber this has been dormant for about 3 weeks. Can please give us a quick update on your plans?
Please note that we currently have multiple pull requests with neural network based pair styles pending and ideally we want to do the final review and processing of all of them for the same patch release so responses from our side may also be slow at times while we focus on ongoing refactoring work in the code base.

@singraber
Copy link
Collaborator Author

First of all, let me thank all of you who already gave valuable suggestions! I am sorry that I did not have the time to process this further for the last weeks. However, I am going to work on the changes starting this week.. since there are no major issues yet I expect that I will have it ready for another round of review until beginning of next week. @akohlmey Does this fit into the planning for the patch release? Is there an approximate date considered for this patch?

@akohlmey
Copy link
Member

First of all, let me thank all of you who already gave valuable suggestions! I am sorry that I did not have the time to process this further for the last weeks. However, I am going to work on the changes starting this week.. since there are no major issues yet I expect that I will have it ready for another round of review until beginning of next week.

No worries. Because of using an external library you PR is ahead of the curve, since there are fewer possible points of conflict. You should probably start with pulling the latest code from upstream into your PR branch and resolve any merge conflicts due to changes that were merged since you submitted your PR.

@akohlmey Does this fit into the planning for the patch release? Is there an approximate date considered for this patch?

This cannot be predicted at the moment. We have some plans for reorganizing packages and it depends on when we implement those. There is a good chance that the NN potentials will be added not to the upcoming patch release, but the one after that (tentatively in early May).

@singraber
Copy link
Collaborator Author

No worries. Because of using an external library you PR is ahead of the curve, since there are fewer possible points of conflict. You should probably start with pulling the latest code from upstream into your PR branch and resolve any merge conflicts due to changes that were merged since you submitted your PR.

Thank you for your suggestion, I will of course make sure that the PR is in no conflict with the master before requesting a review.

This cannot be predicted at the moment. We have some plans for reorganizing packages and it depends on when we implement those. There is a good chance that the NN potentials will be added not to the upcoming patch release, but the one after that (tentatively in early May).

Ok, thanks for the explanation and the perspective!

@akohlmey
Copy link
Member

@sjplimp @athomps this is now ready for final review

@athomps
Copy link
Contributor

athomps commented May 25, 2021

I have a couple of minor changes that I would like to push shortly.

@akohlmey
Copy link
Member

I have a couple of minor changes that I would like to push shortly.

There have been difficulties pushing into this PR despite having the permission to push. I just had to resolve to some unusual tricks to resolve the merge conflict due to the URL changes.

I you have problems, please email me a git diff output and I'll see what I can do.

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.

I approve this PR.

@athomps
Copy link
Contributor

athomps commented May 25, 2021

Done with edits. I only experienced the normal amount of difficulty :-).

@akohlmey
Copy link
Member

Done with edits. I only experienced the normal amount of difficulty :-).

You live a charmed life. Some higher entity must have recently decided to make me suffer for the sins I have committed in my life. 😢

this needs a special treatment when compiling with the MinGW cross-compiler
@akohlmey akohlmey merged commit 2fa389e into lammps:master May 25, 2021
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

5 participants