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

Inconsistency between lammps kc-z and rust kc-z in settings #103

Open
ExpHP opened this issue Jul 10, 2019 · 0 comments
Open

Inconsistency between lammps kc-z and rust kc-z in settings #103

ExpHP opened this issue Jul 10, 2019 · 0 comments
Labels

Comments

@ExpHP
Copy link
Owner

ExpHP commented Jul 10, 2019

This issue has been known since the day the rust reimplementation of KCZ was added. I always assumed there was an easy fix, but after trying to fix it, I've realized it's quite difficult to fix.

I'm creating this issue simply to document why it's stuck like this for the time being, so that I hopefully don't waste my time trying to fix it again.

Why LAMMPS' kc-z and kc-full automatically include REBO:

Basically, LAMMPS' potentials do not compose. Sometimes different information must be encoded in different ways and there's nothing you can do about it. Consider:

  • In pair_style hybrid/overlay rebo kolmogorov/crespi/full, REBO must assign the atom types in order to differentiate between Carbon and Hydrogen.
  • In pair_style hybrid/overlay rebo kolmogorov/crespi/z, Kolmogorov/Crespi must assign the atom types, because it uses them as layers. (Hydrogen simply isn't supported).

(theoretically in this case you can try to encode both element type AND layers in atom type, but... I don't see how well this scales to other cases)

One could say, "well, just take a list of potentials and then hardcode the behavior for accepted combinations." This is possible, but (a) good heavens is it tedious, and (b) even then there's several nasty details that will need to be ironed out. For instance, currently, you can write:

potential:
  rebo:
    omp: true

and this selects the pair_style rebo/omp. But, (a) rebo/omp requires hybrid/overlay/omp, and (b) there are no omp versions of the kolmogorov/crespi potentials, making this ultimately incompatible with kc-z. To fix this, omp might need to be moved to the lammps heading. It's just too much to deal with right now for too little gain.

Why kc-z-new and rebo-new are separate:

Separate is the way they should be, and it's hard for me to picture taking a good thing and ruining it just to be consistent with the LAMMPS potentials.

Quite honestly, code dealing with the KCZ potential has no business worrying about REBO, and this is the solution that best scales towards having more potentials added in the future. For instance, this currently enables the Rust KCZ to be used together with the LAMMPS REBO (e.g. in case we wanted both C(1) continuity in KCZ, and reactivity in REBO).

@ExpHP ExpHP added the wontfix label Jul 10, 2019
ExpHP added a commit that referenced this issue Jul 10, 2019
It has been really bothering me that `kc-z` includes REBO while
`kc-z-new` does not.  I wanted to make things more consistent.

The original goal was to split up the LAMMPS' kc-z and kc-full
potentials so that they did not automatically include REBO terms.
However, that proved to be unworkable.  e.g. it was no longer
clear how atom types were to be decided. See issue #103 for more
details.

So I settled for just moving all of the lammps potentials under
a new potential called `lammps`. This potential is only allowed to
appear once (avoiding what were surely ugly errors when used
multiple times). Furthermore, a warning is presented when `kc-z-new`
is used alone, to suggest adding `nonreactive-rebo`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant