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

Anisotropic Interlayer potential for TMDs and Metal Surfaces #3125

Merged
merged 9 commits into from Feb 17, 2022

Conversation

oywg11
Copy link
Collaborator

@oywg11 oywg11 commented Feb 12, 2022

Summary

Add two anisotropic Interlayer potentials:
1, for transition metal dichalcogenide and its heterostructures
2, for graphene/metal heterojunctions

Related Issue(s)

None

Author(s)

Wengen Ouyang, Wuhan University, w.g.ouyang@gmail.com

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

Yes

Implementation Notes

The new ILPs are added as derived classes of pair ilp/graphene/hbn to avoid code duplication.
The main modifications are the definitions of atomic normal vectors for different materials.

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
https://pubs.acs.org/doi/10.1021/acs.jctc.1c00782
https://pubs.acs.org/doi/10.1021/acs.jctc.1c00622

@akohlmey
Copy link
Member

@oywg11 thanks for your contribution. I had it updated to the latest upstream code and reverted the changes that were undoing the updates from PR #3090 as well as update the new virtual functions accordingly. This also needed some additional changes in the documentation to have it fully integrated and spelling and anchor label issues removed. For the ilp/graphene/hbn pair style I also simplified the per style labels with a local map.
Please check if everything is still working as expected.

@oywg11
Copy link
Collaborator Author

oywg11 commented Feb 16, 2022

@oywg11 thanks for your contribution. I had it updated to the latest upstream code and reverted the changes that were undoing the updates from PR #3090 as well as update the new virtual functions accordingly. This also needed some additional changes in the documentation to have it fully integrated and spelling and anchor label issues removed. For the ilp/graphene/hbn pair style I also simplified the per style labels with a local map. Please check if everything is still working as expected.

@akohlmey Thanks for the improvement. I recompiled the codes and confirmed that they give the same results as before.

Copy link
Contributor

@sjplimp sjplimp left a comment

Choose a reason for hiding this comment

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

my other Q is about the suffixes of the potential files - namely the ones that end in ILP
a) it's unclear to me which ones apply to which ILP package potentials, since we now have several potentials
b) it's also unclear to me why the suffix is ILP instead of ilp which would be consistent with (nearly?) all the other potential files
c) if some of the ILP potential files are specific to certain ILP potentials, then a name like BNCH.ilp.hbn would make sense, otherwise just BNCH.ilp

I realize some ILP files pre-date this PR, but now would be a good time to change the names if Axel agrees.

@akohlmey
Copy link
Member

I realize some ILP files pre-date this PR, but now would be a good time to change the names if Axel agrees.

I had been pondering the same thing. So my first impulse was ask for something similar, but in the interest of having this merged for the patch release I thought it would be better to postpone this. My reason is threefold: 1) most of the pair styles and potential files are for rather specific cases, so it is not likely to have a large library of them (like for eam, tersoff, and similar), 2) when renaming the files for more consistent suffixes it should also be looked into more consistent names for the pair styles, if possible and meaningful, 3) it is probably necessary to do a more general audit of the potential files in the potentials folder.

Bottom line: I am undecided on whether to do this within this PR or later.

@sjplimp
Copy link
Contributor

sjplimp commented Feb 16, 2022

I think it is fine to leave this PR as-is, i.e. ready to merge.

For a future PR on potential dir filenames, I think the key consistency to maintain (which suffix=ILP breaks) is that the suffix of the potential file be an exact match to the pair style name. E.g. eam or eam.alloy. All lowercase, like the pair style names.
So that if someone browses the potentials dir and wonders what this Carbon potential file is good for, they can use the suffix to go to the pair style doc page directly. No ambiguity.

If some potential files can be used with multiple pair styles (e.g. ILP package variants), then possibly suffix = base name of multiple pair styles, and the pair style doc page can clarify that these 3 pair style variants use the same potential files.

@sjplimp
Copy link
Contributor

sjplimp commented Feb 16, 2022

Also, changing pair style names to be more informative is a separate issue. Anytime that is done, the corresponding potential file names should be changed accordingly.

@akohlmey
Copy link
Member

I think it is fine to leave this PR as-is, i.e. ready to merge.

I won't be able to merge it, though, for as long as your request for changes is blocking it.

@oywg11
Copy link
Collaborator Author

oywg11 commented Feb 17, 2022

I think it is fine to leave this PR as-is, i.e. ready to merge.

For a future PR on potential dir filenames, I think the key consistency to maintain (which suffix=ILP breaks) is that the suffix of the potential file be an exact match to the pair style name. E.g. eam or eam.alloy. All lowercase, like the pair style names. So that if someone browses the potentials dir and wonders what this Carbon potential file is good for, they can use the suffix to go to the pair style doc page directly. No ambiguity.

If some potential files can be used with multiple pair styles (e.g. ILP package variants), then possibly suffix = base name of multiple pair styles, and the pair style doc page can clarify that these 3 pair style variants use the same potential files.

@sjplimp Thanks for the suggestion. The reason why I didn't follow the rules is that the potential file names would be super long, for instance, BNCH.ILP should be named as BNCH.ilp.graphene.hbn. So I used the element to separate the potential files (i.e, the prefix). Another reason is that the interlayer potentials I submitted actually share the same formula. The main difference between them is the definition of the atomic normal vectors for different materials. If you think the long potential file name is not an issue, I'm also fine with renaming all the potential files.

@oywg11
Copy link
Collaborator Author

oywg11 commented Feb 17, 2022

For your information, here I list the applicable scope of three ILPs:
1, pair ilp/graphene/hbn is applicable to the following heterojunctions: graphene/graphene, hBN/hBN, graphene/hBN and hydrocarbons/graphene, hydrocarbons/hBN;
2, pair ilp/tmd is applicable to the following heterojunctions: MX2/MX2 (M=Mo,W; X=S,Se,Te), graphene/MX2 and hBN/MX2;
3, pair saip/metal is applicable to the following heterojunction: graphene/metal(=Au,Cu,Ag,Ni) and hBN/metal(=Au,Cu,Ag,Ni).

@sjplimp
Copy link
Contributor

sjplimp commented Feb 17, 2022

@oywg11 Long potential file names is not a problem. As I mentioned previously, the more important consideration is clarity for users as to which potential file to use with which pair style.

But we can adjust the file names later. I'll appove this PR. Thanks again for submitting it to LAMMPS.

@akohlmey akohlmey merged commit 45b92e5 into lammps:develop Feb 17, 2022
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

3 participants