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

changes the values of some parameters in REBO in accordance to the or… #986

Closed
wants to merge 195 commits into from

Conversation

@CF17
Copy link

commented Jul 3, 2018

…iginal Brenner paper

Purpose

This pull request is fixing a bug reported in #726. The parameters currently implemented in LAMMPS is using the AIREBO parameters and not the REBO parameters as published in J. Phys.: Condens. Matter 14 (2002) 783. This was partially fixed by Favata and coworkers but some parameters remained to be changed. In addition additional parameters given in the potential file CH.airebo are slightly different than those published by Brenner. The potential CH.rebo was then created.
closes #726

Author(s)

Cyril Falvo (Université Paris Sud)

Backward Compatibility

This fix do not break Backward compatibility but new calculation will give different results.

Implementation Notes

Comparison of the atomization energies (in eV) given in J. Phys.: Condens. Matter 14 (2002) 783 with the values computed with the current version of REBO potential and the modified REBO potential (REBO')

CAS number Brenner REBO REBO' molecule name
2465-56-7 -8.4693 -8.4693 -8.4693 Methylene
2229-07-4 -13.3750 -13.3750 -13.3750 Methyl radical
74-82-8 -18.1851 -18.1851 -18.1851 Methane
2122-48-7 -11.5722 -11.5722 -11.5722 Ethynyl radical
74-86-2 -17.5651 -17.5651 -17.5651 Acetylene
74-85-1 -24.4077 -24.5284 -24.4077 Ethylene
2025-56-1 -26.5601 -26.5879 -26.5601 Ethyl radical
74-84-0 -30.8457 -30.8457 -30.8457 Ethane
2781-85-3 -28.2589 -28.3610 -28.2589 Cyclopropene
463-49-0 -30.2392 -30.3511 -30.2392 Allene
74-99-7 -30.3076 -30.3076 -30.3076 Propyne
75-19-4 -36.8887 -36.8887 -36.8887 Cyclopropane
115-07-1 -37.3047 -37.5116 -37.3047 Propene
2143-61-5 -39.3042 -39.3320 -39.3042 n-Propyl radical
2025-55-0 -39.4601 -39.5535 -39.4602 Isopropyl radical
74-98-6 -43.5891 -43.5891 -43.5891 Propane
822-35-5 -42.1801 -42.3536 -42.1797 Cyclobutene
106-99-0 -43.0035 -43.3977 -43.0035 1,3-Butadiene
590-19-2 -43.1367 -43.3321 -43.1367 1,2-Butadiene
107-00-6 -43.0510 -43.0510 -43.0510 1-Butyne
503-17-3 -43.0501 -43.0501 -43.0501 2-Butyne
287-23-0 -49.7304 -49.8986 -49.8986 Cyclobutane
106-98-9 -50.0487 -50.2557 -50.0486 1-Butene
590-18-1 -50.2017 -50.4951 -50.2016 2-Butene, (Z)-
65114-21-8 -52.0451 -52.0728 -52.0451 i-C4H9
106-97-8 -56.3326 -56.3325 -56.3325 Butane
75-28-5 -56.3309 -56.3307 -56.3307 Isobutane
504-60-9 -55.9025 -56.3731 -55.9025 1,3-Pentadiene
591-93-5 -56.5078 -56.9220 -56.5079 1,4-Pentadiene
142-29-0 -57.1119 -57.3895 -57.1121 Cyclopentene
591-95-7 -58.7350 -56.0762 -55.8807 1,2-Pentadiene
591-96-8 -58.8900 -56.3130 -56.0342 2,3-Pentadiene
287-92-3 -63.6443 -63.6455 -63.6455 Cyclopentane
109-68-2 -62.9456 -63.2392 -62.9456 2-Pentene
563-46-2 -62.9658 -63.0254 -62.9658 2-Methyl-1-butene
513-35-9 -63.1109 -63.2642 -63.1188 2-Butene, 2-methyl-
109-66-0 -69.0761 -69.0760 -69.0760 Pentane
78-78-4 -69.0739 -69.0741 -69.0741 Butane, 2-methyl-
463-82-1 -69.0614 -69.0614 -69.0614 Neopentane
71-43-2 -59.3066 -60.2313 -59.3096 Benzene
110-82-7 -76.4606 -76.4606 -76.4606 Cyclohexane

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

@akohlmey akohlmey requested review from sjplimp and athomps Jul 5, 2018

@ExpHP

This comment has been minimized.

Copy link

commented Sep 7, 2018

I wonder if this impacts the validity of the parameters for kolmogorov/crespi/z, which, if I understand correctly, was scaled to match the bond length of the LAMMPS implementation of REBO?

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

@akohlmey

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

@oywg11 can you please check, if this PR affects the accuracy of your contributed potentials?

@oywg11

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2018

@oywg11 can you please check, if this PR affects the accuracy of your contributed potentials?

@akohlmey No problem. I will check it ASAP.

@oywg11

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2018

@oywg11 can you please check, if this PR affects the accuracy of your contributed potentials?

@akohlmey No problem. I will check it ASAP.

@akohlmey Hi Axel, I checked the intralayer potential (original REBO vs. modified REBO) with the interlayer potential (pair ilp/graphene/hbn) by minimizing the bilayer graphene with fire algorithm. When reaching the optimal configuration, the interlayer distance for REBO+ILP and modified REBO+ILP is 3.3694 A and 3.3695 A, respectively. The difference between interlayer energy is 0.004 meV/atom. The corresponding intralayer (REBO) energy is -7.39497 eV/atom and -7.79080 eV/atom, respectively. According to the above data, I think this PR does not affect the accuracy of my contributed potentials. Do you think we need to do more tests?

@oywg11

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2018

@akohlmey Another thing may worth to mention is that the FIRE algorithm finds the minimum much faster (x8) for the modified REBO potential than the original REBO potential.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

@oywg11 i think these differences are acceptable. i expected only small differences in the first place, but it is good to have some confirmation.

in re: FIRE, this can be random (small changes in the forces can trigger a different path to the minimum in a high-dimensional optimization problem), or due to the corrected REPO parameters leading to a smoother potential surface.

@akohlmey akohlmey self-requested a review Sep 25, 2018

@akohlmey akohlmey self-assigned this Sep 27, 2018

@akohlmey akohlmey removed their request for review Sep 27, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

@sjplimp @athomps am reviewing older pull requests currently, and this one is still on the TODO list. can you please add a review and either approve or suggest changes or point out problems. thanks.

@@ -30,9 +30,263 @@ void PairREBO::settings(int narg, char **arg)

cutlj = 0.0;
ljflag = torflag = 0;
PCCf_2_0 = -0.0276030;

This comment has been minimized.

Copy link
@ExpHP

ExpHP Nov 9, 2018

Why is this still here? Can't the PCCf_2_0 member just be removed, with the value inlined back into pair_airebo.cpp?

@akohlmey

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

@athomps
Copy link
Contributor

left a comment

If I understand correctly, this change eliminates the need for the PCCf_2_0 base class member variable.

pair_airebo.h: double PCCf_2_0;

If so, this should be fully removed from both PairAIREBO and PairREBO source files.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

@athomps , @ExpHP is commit a625f1b the change, that you are looking for?
if yes, please approve and i will merge it ASAP. it will be nice to get this one finally sorted out.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

Since this is based on a LAMMPS version from several months ago, i will do a rebase to the current master and push this into a branch in the LAMMPS project repository. That we people can submit pull requests to that specific branch. for further improvements. as far as i can see, there should be the following:

  • update to the REBO/AIREBO pair style documentation
  • a check for telling the CH.rebo potential file apart from CH.airebo and printing a warning, if there is a mismatch.

@akohlmey akohlmey closed this Nov 9, 2018

@akohlmey akohlmey reopened this Nov 9, 2018

@ExpHP

This comment has been minimized.

Copy link

commented Nov 12, 2018

Now that I've been working on my own REBO implementation, one thing I wonder is whether perhaps the changes to PCC[2][0], PCC[1][1], and PCC[0][2] should be incorporated into AIREBO when torsionflag = 0? After all, Stuart describes the purpose of these changes as to counteract additional energies from torsion, and so torsionflag = 0 seems somewhat ill-defined at present.

After that, I think pair_style rebo could once again be made to be actually equivalent to pair_style airebo 0 0?

This is just spitballing though. I suspect people may find the current definition useful for computing the value of the "torsion term," even if it does not reflect the actual change in potential due to accounting for torsion.

@akohlmey akohlmey assigned sjplimp and unassigned akohlmey Nov 16, 2018

junghans and others added some commits Dec 20, 2018

"mixing rule" for lambda in init_one() method
The values of the lambda[i][j] were equal to zero and different from lambda[i][i] when the user was not using explicit pair_coeff commands for the i-j pairs in the input script. The "mixing rule" included in this file is the same with the one in the pair_lj_cut_coul_cut_soft.cpp and pair_lj_cut_coul_long_soft.cpp files.
Merge pull request #1268 from junghans/cmake_install
cmake: adjust install location of FindLAMMPS.cmake
Merge pull request #1266 from junghans/cmake_doc
doc: add CMAKE_VERBOSE_MAKEFILE and CXX_COMPILER_LAUNCHER
Merge pull request #1267 from evoyiatzis/patch-1
add "mixing rule" check for inconsistent lambda in init_one() method of lj/cut/soft
Merge pull request #1265 from junghans/linux_doc
Update pre-packaged LAMMPS for linux docs
Merge pull request #1239 from jrgissing/bond/react-delete_atoms
fix bond/react: allow deleting atoms
Merge pull request #1243 from athomps/snap-foundtest
Removed redundant element list from pair_coeff snap syntax
Merge pull request #1255 from stanmoore1/sllod
Change fix nvt/sllod to allow suffix styles of fix deform
Merge pull request #1259 from athomps/npt-econs-varv
Fix two problems with NPT: volume fluctuations (with iso, previously with aniso/tri) and energy conservation (with aniso/tri)
Merge pull request #1262 from evoyiatzis/evoyiatzis-patch-1
Addition of extract() method for class2 pair potentials
Merge pull request #1254 from akohlmey/remove-reax-meam
Remove REAX and MEAM packages
@akohlmey

This comment has been minimized.

Copy link
Member

commented Dec 29, 2018

@sjplimp @athomps how should be proceed with this?

akohlmey added some commits Dec 30, 2018

Merge branch 'github-rebo-bugfix' of https://github.com/CF17/lammps i…
…nto test-rebo

# Conflicts:
#	examples/airebo/log.23Jun17.airebo-m.g++.1
#	examples/airebo/log.23Jun17.airebo-m.g++.4
#	examples/airebo/log.23Jun17.airebo.g++.1
#	examples/airebo/log.23Jun17.airebo.g++.4
#	examples/airebo/log.27Nov18.airebo-m.g++.1
#	examples/airebo/log.27Nov18.airebo-m.g++.4
#	examples/airebo/log.27Nov18.airebo.g++.1
#	examples/airebo/log.27Nov18.airebo.g++.4
#	examples/airebo/log.29Jun18.airebo-m.g++.1
#	examples/airebo/log.29Jun18.airebo-m.g++.4
#	examples/airebo/log.29Jun18.airebo.g++.1
#	examples/airebo/log.29Jun18.airebo.g++.4

@akohlmey akohlmey dismissed stale reviews from athomps and themself via 41ccf83 Dec 30, 2018

@CF17 CF17 requested review from junghans, rbberger and stanmoore1 as code owners Dec 30, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

@junghans , @rbberger , @stanmoore1 You can ignore the review requests. Those were auto-generated from the code owners file due to me merging in master to resolve merge conflicts and update the PR.

@athomps

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

I think it would be a bad idea to combine rebo and airebo into a single pair_style command. Even though the models are very similar and share a lot of source code under the hood, it is less confusing for users if they have distinct names.

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 4, 2019

@sjplimp @athomps this has been sitting around and waiting for you to make a decision for a very long time. either we merge this now or dismiss it.

@athomps

athomps approved these changes May 4, 2019

Copy link
Contributor

left a comment

I can't review 924 files, so I am just going to approve it.

@akohlmey akohlmey referenced this pull request May 4, 2019
9 of 9 tasks complete
@akohlmey

This comment has been minimized.

Copy link
Member

commented May 4, 2019

I can't review 924 files, so I am just going to approve it.

we can do this better. i've updated the branch of the PR to current master and resubmitted it as a new PR #1452. now only the changes from the branch are visible and a proper review should be possible.

@athomps

This comment has been minimized.

Copy link
Contributor

commented May 4, 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.