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

E3B water model #1428

Merged
merged 20 commits into from May 14, 2019

Conversation

@sstrong99
Copy link
Collaborator

commented Apr 16, 2019

Summary
Add a pair style to implement the e3b (Explicit Three Body) water model. As well as a related compute that extracts the several potential energy contributions of this pair style.

Author(s)
Steven E Strong (U Chicago) StevenE.Strong at gmail dot com
Nicholas J Hestand (U Chicago)

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).

Implementation Notes

Tested pair style for energy conservation. Also tested against potential energy and density benchmarks in the papers describing E3B, using the original GROMACS code for those papers.

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted. Delete lines that don't apply

  • 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
  • A package specific README file has been included or updated
  • One or more example input decks are included
@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@sstrong99 thank you for your contribution. There are some compile issues as flagged by the PR tests:

../pair_e3b.cpp:667:7: note: suggested alternative:
In file included from ../domain.h:17:0,
                 from ../pair_e3b.cpp:28:
/usr/include/c++/7/cmath:639:5: note:   'std::isnan'
     isnan(_Tp __x)
     ^~~~~

It looks like you need to use std::isnan. There is also an issue with the docs:

! Package pdftex.def Error: File `Eqs/e3b.jpg' not found: using draft setting.

See https://ci2.lammps.org/job/lammps/job/pull-requests/job/build-docs-pr/233/console.

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2019

Ok, sorry about that. I didn't get that error on my machine. I did see that the PR test failed, but I didn't know how to see what the error was so that I could fix it. I've fixed the isnan error. Regarding the e3b.jpg error: I had read here that the jpg files are auto-created from the .tex files. The e3b.tex file is already included in doc/src/Eqs

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 19, 2019

I have converted the e3b.tex output to a jpg myself and committed it, in case that documentation is wrong.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@sstrong99 thanks, I'm not sure why the automatic tex to jpg conversion didn't work...

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 22, 2019

It seems like the doc compile is still failing here. I'm not sure why. It works fine on my machine

@akohlmey

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

It seems like the doc compile is still failing here. I'm not sure why. It works fine on my machine

The problem is not in the doc build (make html), but in the spell-checking (make spelling). It looks like you need to as some more false positives to doc/utils/sphinx-config/false_positives.txt

@sstrong99 sstrong99 requested a review from rbberger as a code owner Apr 24, 2019

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 24, 2019

Ok. I added those false positives. The doc build still fails. Any ideas?

@akohlmey akohlmey self-assigned this Apr 25, 2019

akohlmey added some commits Apr 25, 2019

@akohlmey akohlmey requested review from sjplimp, athomps and stanmoore1 Apr 25, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@sstrong99 looks like i found the issue that triggered the doc test failure and made an update for the legacy documentation.

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 25, 2019

Great, thanks!

single_enable = 0;
restartinfo = 0;
one_coeff = 1;
manybody_flag = 1;

This comment has been minimized.

Copy link
@akohlmey

akohlmey Apr 25, 2019

Member

@sstrong99 is there a particular reason, why you set the manybody_flag to 1 here? it is meant to trigger a warning when a manybody potential (where bonded interactions are implicit) is used with explicit bonds. in your examples the explicit bonds, however, are intentional, so the warning may confuse people.

This comment has been minimized.

Copy link
@sstrong99

sstrong99 Apr 25, 2019

Author Collaborator

No, I only set that because it seemed most consistent. But it looks like the flag isn't really used for anything except that warning. So I agree that it doesn't need to be set. I will fix that.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@sstrong99 i've modified the example input to be more in line with other LAMMPS examples, and in particular used a data file to initialize the system, so i can compare if serial and parallel runs give the same results (they do). however, when running this input with valgrind's memcheck, i get lots of errors about accessing unitialized data, which point to the array del3. can you please have a closer look at that?

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 25, 2019

Yes, I will figure out what's going on. I am going on vacation tomorrow though, and wont have a chance to do that until I get back. Maybe around May 7 or so.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Yes, I will figure out what's going on. I am going on vacation tomorrow though, and wont have a chance to do that until I get back. Maybe around May 7 or so.

no problem. i just cancel the review requests and label it work_in_progress for now.

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2019

@akohlmey I have fixed the uninitialized data errors. The del3 array was never initialized because when it is used, it is always multiplied by the corresponding value in either the exps or fpair3 arrays. These arrays are always set or initialized to zero on every call to compute(), so I didn't initialize del3. Now I initialize del3 when it is allocated to silence the errors, but I don't reinitialize them on every call to compute() since exps and fpair3 are initialized.

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@sstrong99 thanks for the update. i have one more question: when taking a closer look at how compute pe/e3b is implemented, i noticed, that this functionality could be folded into compute pair by allocating and populating the Pair::pvector array with the data and setting npair to 4. what do you think? is this desirable from your side? it would help maintaining the code. i can make the necessary changes.

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

@akohlmey I incorporated compute_pe_e3b into compute_pair. Thank you for that suggestion.

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@akohlmey I incorporated compute_pe_e3b into compute_pair. Thank you for that suggestion.

thanks for the update. sorry for noticing this only so late. LAMMPS has become such a big package, it often takes multiple passes of reviewing contributions to notice all possible improvements and requirements for integration.

@sstrong99

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

No problem. Let me know if there is anything else.

@stanmoore1
Copy link
Contributor

left a comment

Looks OK to me.

@akohlmey akohlmey merged commit 60f6c3f into lammps:master May 14, 2019

6 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/cmake/cmake-serial-pr head run ended
Details
lammps/pull-requests/kokkos-omp-pr 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

@sstrong99 sstrong99 deleted the sstrong99:e3b branch May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.