Skip to content

pair_style d3 - D3 dispersion corrections#4410

Merged
akohlmey merged 15 commits into
lammps:developfrom
soniasalomoni:pair_d3
Jan 14, 2025
Merged

pair_style d3 - D3 dispersion corrections#4410
akohlmey merged 15 commits into
lammps:developfrom
soniasalomoni:pair_d3

Conversation

@soniasalomoni
Copy link
Copy Markdown
Collaborator

Summary

Implementation of the D3 correction scheme as pair-style d3, in the EXTRA-PAIR package. It should typically be used with an ML potential through pair_style hybrid/overlay.

Related Issue(s)

N/A

Author(s)

Sonia Salomoni, Sorbonne Université (sonia.salomoni AT sorbonne-universite DOT fr)
Arthur France-Lanord, CNRS (arthur.france-lanord AT cnrs DOT fr)

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

Implementation Notes

The code was validated using the original Fortran DFT-D3 implementation and the CP2K D3 implementation.

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

Currently working on a kokkos implementation, which we will push later.

@arthurfl
Copy link
Copy Markdown
Collaborator

Some of the unit tests fail because of the expected precision, e.g. from the Linux tests:

324: Google Test trace:
324: /home/runner/work/lammps/lammps/unittest/force-styles/test_main.cpp:56: EXPECT_FORCES: init_forces (newton on)
324: /home/runner/work/lammps/lammps/unittest/force-styles/test_main.cpp:65: Failure
324: Expected: (err) <= (epsilon)
324:   Actual: 4.912891623658158e-09 vs 9.9999999999999994e-12

Also, the reference data was generated using a MacOS build, with the following configuration:

-- <<< Build configuration >>>
   LAMMPS Version:   20241119 patch_19Nov2024-56-ga78aee5731
   Operating System: Darwin
   CMake Version:    3.29.2
   Build type:       RelWithDebInfo
   Install path:     /Users/afl/.local
   Generator:        Unix Makefiles using /usr/bin/make
-- Enabled packages: EXTRA-COMPUTE;EXTRA-DUMP;EXTRA-FIX;EXTRA-MOLECULE;EXTRA-PAIR;MANYBODY;MISC;MOLECULE
-- <<< Compilers and Flags: >>>
-- C++ Compiler:     /Library/Developer/CommandLineTools/usr/bin/c++
      Type:          AppleClang
      Version:       14.0.3.14030022
      C++ Standard:  11
      C++ Flags:     -O2 -g -DNDEBUG
      Defines:       LAMMPS_SMALLBIG;LAMMPS_MEMALIGN=64;LAMMPS_OMP_COMPAT=4;LAMMPS_JPEG;LAMMPS_PNG;LAMMPS_GZIP;LAMMPS_FFMPEG
-- C compiler:       /Users/afl/opt/miniconda3/bin/arm64-apple-darwin20.0.0-clang
      Type:
      Version:
      C Flags:      -ftree-vectorize -fPIC -fPIE -fstack-protector-strong -O2 -pipe -isystem /Users/afl/opt/miniconda3/include -O2 -g -DNDEBUG
-- <<< Linker flags: >>>
-- Executable name:  lmp
-- Executable linker flags: -Wl,-pie -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-rpath,/Users/afl/opt/miniconda3/lib -L/Users/afl/opt/miniconda3/lib
-- Shared library flags:    -Wl,-pie -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-rpath,/Users/afl/opt/miniconda3/lib -L/Users/afl/opt/miniconda3/lib
-- <<< MPI flags >>>
-- MPI_defines:      MPICH_SKIP_MPICXX;OMPI_SKIP_MPICXX;_MPICC_H
-- MPI includes:     /opt/homebrew/Cellar/open-mpi/5.0.3_1/include
-- MPI libraries:    /opt/homebrew/Cellar/open-mpi/5.0.3_1/lib/libmpi.dylib;
-- <<< Building Unit Tests >>>

Are there any guidelines on build, compiler flags etc. to generate reference data? Or should we simply relax the precision parameter?

@sjplimp
Copy link
Copy Markdown
Contributor

sjplimp commented Dec 10, 2024

quick comment - "d3" is not a descriptive enough name for general users - maybe dispersion/d3 ?

@akohlmey
Copy link
Copy Markdown
Member

akohlmey commented Dec 11, 2024

Are there any guidelines on build, compiler flags etc. to generate reference data?

The Apple-Clang compilers are always a bit funny when compiling with high optimization. You get more consistent results when turning off optimization, e.g. with CMAKE_BUILD_TYPE=Debug, and also running on Linux using gcc instead of clang. The unit tests are deliberately very sensitive and 1e-11 is a very tight epsilon that will only work well for simple fully analytically potentials that have a very smooth potential function and do not use tabulation or interpolation.

But this is a minor issue, a quick look at the submitted code shows, that we need to make some modifications to have it better aligned with LAMMPS' coding conventions and also add some maintenance related changes. @soniasalomoni @arthurfl do you prefer to make those changes yourself and have me point them all out in detail, or are you ok with me making the simpler ones myself and only comment on the bigger changes required? The first option is less work for me but less helpful in the long run, in case you plan to do further submissions and updates in the future. Please let me know.

@akohlmey akohlmey self-assigned this Dec 11, 2024
@soniasalomoni
Copy link
Copy Markdown
Collaborator Author

Thanks for the feedback, option 2 would be best, if it’s not too much extra work for you.

@akohlmey
Copy link
Copy Markdown
Member

@soniasalomoni @arthurfl this pull request is now consistent with the minimum requirements for including it to the distribution. Specifically, the following issues were addressed:

  • update of src/.gitignore
  • full integration into the manual including references from the pair style and command summary lists
  • rename to dispersion/d3 and corresponding changes to class and file names
  • create the parameter arrays as static constexpr double arrays in stead of local const arrays initialized from defines. Those local arrays would blow up the stack requirements that would cause failures on Windows.
  • formatting and spelling corrections as well as spellcheck false positive updates for the manpage
  • increase unit test epsilon so it passes the test on all tested platforms. The use of tables makes potentials rather noisy and thus requires such large epsilon
  • when testing the native Windows compilation to address the unexpected unit test failures, I also ran into a rounding ambiguity in a different unit tests, which is now fixed as well.

Before we merge this, please let me know if you have any concerns and worries about the changes made or if you can spot any errors, where I may have misunderstood your intentions.

A couple more questions:

  • Do you have (or can quickly create) a small example input that would demonstrate the typical use of this pair style? Doesn't have to represent actual research, but just a system with a few 100 atoms run for a small number of steps.
  • Is there a publication that specifically addresses the issue of why someone should use this pair style? For that a citation reminder could be added. The manpage only seems to provide references of the D3 correction parameters in general.

@akohlmey akohlmey requested a review from jmgoff December 18, 2024 10:59
@arthurfl
Copy link
Copy Markdown
Collaborator

Thanks a lot for the update. We'll add an example, although to be meaningful, and because of the way this pair_style is intended to be used, it will depend on an ML potential (probably a pace pair_style in the example). Concerning the reference, I am not aware of any work focusing on the benefit of separating the reference DFT and the dispersion correction rather than training directly on both. There are however many papers in which this strategy is used, usually with D2, since as a pure two-body correction, it can be easily tabulated. We'll add something along these lines to the docs.

@arthurfl
Copy link
Copy Markdown
Collaborator

@akohlmey regarding the example folder, should we go for examples/dispersion, examples/DISPERSION, examples/PACKAGES/extra-pair, or something else?

@akohlmey
Copy link
Copy Markdown
Member

@akohlmey regarding the example folder, should we go for examples/dispersion, examples/DISPERSION, examples/PACKAGES/extra-pair, or something else?

I think most consistent with current practices would be either examples/PACKAGES/dispersion or examples/PACKAGES/dispersion-d3.

Copy link
Copy Markdown
Collaborator

@megmcca megmcca left a comment

Choose a reason for hiding this comment

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

looks like it's in good shape. i've successfully run both of the example files using the updated README instructions.

@akohlmey akohlmey merged commit bbb7d86 into lammps:develop Jan 14, 2025
s8 = 0.9147;
a2 = 5.0570;
break;
s6 = 0.64;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@soniasalomoni @arthurfl one question about this block.

The s6 = 0.64; line is never executed. Is this expected and it can be removed (it triggers warnings with static code analysis)?
Or is the "break;" statement misplaced and should be moved one line down?

Please advise

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Second option, the "break;" statement should be moved one line down. Thanks for the heads-up

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

Projects

Development

Successfully merging this pull request may close these issues.

5 participants