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

Develop native support for KIM simulator models #1440

Merged
merged 121 commits into from Jul 30, 2019

Conversation

@ellio167
Copy link
Collaborator

commented May 1, 2019

Summary

This PR will create built-in support for KIM Simulator Models

Author(s)

Ryan S. Elliott (U. Minnesota)
Ellad B. Tadmor (U. Minnesota)
Axel Kohlmeyer (Temple U)

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

Most of the new commands work by essentially generating LAMMPS commands, either individually or from a file and feeding them to the LAMMPS instance. For clarity, they will generated begin/end markers to show the generated LAMMPS commands like in the following example log output segments:

kim_init        SW_StillingerWeber_1985_Si__MO_405512056662_005 real
#=== BEGIN kim-init ==========================================
units real
#=== END kim-init ============================================
kim_interactions Si
#=== BEGIN kim_interactions ==================================
pair_style kim SW_StillingerWeber_1985_Si__MO_405512056662_005
pair_coeff * * Si 
#=== END kim_interactions ====================================
Start devel of native support for KIM simulator models
* CMake change to use KIM-API SimulatorModels branch
* Minimal changes to pair_kim to illustrate use of KIM API
  interface. Only c++ interface is implemented for development.
* Added example input: in.kim.simulator-model
@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 1, 2019

@akohlmey, I think this is ready for you to take a look at when you have time.

Thanks, Ryan

cc: @tadmor

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@akohlmey, I think this is ready for you to take a look at when you have time.

Thanks, Ryan

cc: @tadmor

@ellio167
been starting to integrate my modifications but progress is slow due to my personal situation. i noticed the example input creates references to the element Ar when printed, while the metadata file only references Si.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

Hi @akohlmey , Thanks for looking at it.

Indeed, the example was hacked together as fast as possible without regard to consistency. A real example would have pair-coeff * * Si as well as other changes (mass, lattice type/spacing, etc.). I believe the example file is an exact copy of one of the other kim example files with just the pair_style kim ... line changed.

akohlmey added some commits May 19, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@ellio167 been working my way through exploring the simulator model API. When i call GetNumberOfSimulatorFields(& int); it sets the provided int to 0. Shouldn't this be >0 and then i would loop over GetSimulatorFieldMetadata() to get the categories like 'model-defn', 'model-init', and 'units'?

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

@akohlmey Correct, you should get something >0;

The process is:

  • create the SimulatorModel object
  • call AddTemplateMap() to add any simulator-specific mappings that should be search-and-replaced.
  • call CloseTemplateMap() to indicate that all mapping are defined and the fields should be processed.
  • access the processed fields with GetNumberOfSimulatorFields(), GetNumberOfSimulatorFields(), and GetSimulatorFieldLine()

Only after the call to CloseTemplateMap are the processed fields available. So, I suspect that you are missing a call to CloseTemplateMap()

akohlmey added some commits May 21, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Only after the call to CloseTemplateMap are the processed fields available. So, I suspect that you are missing a call to CloseTemplateMap()

thanks. yes, i was calling those APIs too early. now after re-arranging things it is working as expected. will have to re-think my strategy a little bit, but it should work nevertheless.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

You could use the ClearTemplateMap(), AddTemplateMap(), CloseTemplateMap() cycle multiple times, if it would be helpful:

Early on, just CloseTemplateMap() and access fields that don't depend on "atom-type-sym-list". Then, later on, ClearTemplageMap(), add the "atom-type-sym-list" mapping, and CloseTemplateMap() again....

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@ellio167 i think i have now the basic steps implemented to instantiate a custom proxy pair style based on the simulator model info and pushed those into your branch. the next step is now to make all the APIs of the Pair class functional by forwarding them to the proxy class pointed to by PairKIM::simulator_class instead of calling the KIM model. This would have to be done in a similar fashion as in PairHybrid but simpler (since there is only one embedded Pair class instance and no mapping is needed (unless PairKIM would be used itself in a hybrid pair style).

Please have a look and add comments where needed.

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@ellio167 FYI, your repo contains two bogus tags 'rm' and 'list'. please delete them. thanks.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

@akohlmey 11407a1 is there a lammps clang-format configuration file or something similar for this style?

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@akohlmey 11407a1 is there a lammps clang-format configuration file or something similar for this style?

come to think of it. providing this would be a good talking point for the LAMMPS workshop, either in the developer session or by talking with @sjplimp directly. if we had such a configuration that he is ok with, we could provide instructions to run it as a commit hook and then code committed by people using this hook would be automatically reformatted to spec.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

@akohlmey 11407a1 is there a lammps clang-format configuration file or something similar for this style?

come to think of it. providing this would be a good talking point for the LAMMPS workshop, either in the developer session or by talking with @sjplimp directly. if we had such a configuration that he is ok with, we could provide instructions to run it as a commit hook and then code committed by people using this hook would be automatically reformatted to spec.

Agreed; many of these conventions are likely supported by clang-format. Even if not, the advantages of having a the consistent auto-formatting is worth, in my opinion, compromising on a few formatting preferences.

I have a clang-format configuration file and notes about how to setup the git hook in the kim-api repo...

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@akohlmey 11407a1 is there a lammps clang-format configuration file or something similar for this style?

come to think of it. providing this would be a good talking point for the LAMMPS workshop, either in the developer session or by talking with @sjplimp directly. if we had such a configuration that he is ok with, we could provide instructions to run it as a commit hook and then code committed by people using this hook would be automatically reformatted to spec.

This could be a nice project for a student intern: devise an optimal clang-format configuration file that results in minimal changes to a set of selected LAMMPS source files (that are determined to define the style best), e.g. using a genetic algorithm.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

@ellio167 i think i have now the basic steps implemented to instantiate a custom proxy pair style based on the simulator model info and pushed those into your branch.

Thanks for the work! This looks like a great start. More below.

the next step is now to make all the APIs of the Pair class functional by forwarding them to the proxy class pointed to by PairKIM::simulator_class instead of calling the KIM model. This would have to be done in a similar fashion as in PairHybrid but simpler (since there is only one embedded Pair class instance and no mapping is needed (unless PairKIM would be used itself in a hybrid pair style).

Please have a look and add comments where needed.

  • A minor issue: The "validate species selection" check seems to be incorrect to me.

  • There are cases where there are more than just "pair_style" and "pair_coeff" commands in the "model-defn" field. Currently, these include commands like "comm_modify" and "fix combqeq". So there would need to be an additional case where we call "input->one()" for these lines.

  • There are cases where the pair style is "hybrid/overlay". Is that a problem?

  • There are cases where the "model-init" field contains the "atom_style" command. It looks to me like this would fail since it generally comes after the "create_box" or similar command. I guess these commands need to be discovered much earlier in the input. I would be interested in your thoughts on how to handle this.

  • We are already starting to experiment with bonded models. So, for example we have a case where the fields are given by:

      "model-init": "atom_style full",
      "model-defn": [
        "kspace_style pppm 1.0e-2",
        "pair_style lj/charmm/coul/long 8.0 8.2",
        "bond_style harmonic",
        "angle_style charmm",
        "dihedral_style charmm",
        "improper_style harmonic",
        "special_bonds charmm",
        "pair_modify mix arithmetic",
        "include @<parameter-file-1>@"
      ]

This is clearly taking things to the next level. At this point, it would be helpful to just know if you see any way forward with this sort of case? It seems to me that the answer might be connected to the previous bullet too.

@akohlmey

This comment has been minimized.

Copy link
Member

commented May 22, 2019

  • There are cases where the "model-init" field contains the "atom_style" command. It looks to me like this would fail since it generally comes after the "create_box" or similar command. I guess these commands need to be discovered much earlier in the input. I would be interested in your thoughts on how to handle this.
  • We are already starting to experiment with bonded models. So, for example we have a case where the fields are given by:
      "model-init": "atom_style full",
      "model-defn": [
        "kspace_style pppm 1.0e-2",
        "pair_style lj/charmm/coul/long 8.0 8.2",
        "bond_style harmonic",
        "angle_style charmm",
        "dihedral_style charmm",
        "improper_style harmonic",
        "special_bonds charmm",
        "pair_modify mix arithmetic",
        "include @<parameter-file-1>@"
      ]

This is clearly taking things to the next level. At this point, it would be helpful to just know if you see any way forward with this sort of case? It seems to me that the answer might be connected to the previous bullet too.

i think for this, it is not ideal to embed this processing into the pair_style kim command. it would be better, similar than with the kim-query command, have a kim-model XXXX or kim-simulator-model XXXX command. then it could have an option init and an option defn and process the corresponding sections by issuing all commands directly. that is do not transparently hide the processing inside the kim pair style with proxy classes, but be more like a pre-processing step, there the kim-simulator-model XXXX init would be the pre-system generation part and the kim-simulator-model XXXX defn would be the post box creation part. it would probably also be a simpler implementation, except for how to map the atom types. haven't thought about how to do that.

it would be extremely helpful at this point, if you could provide some more simulator model input examples demonstrating the multiple variants of what you are looking into. if you agree with trying out my suggestion for the alternate command instead of embedding things into pair_style kim, i'll see what i can do in a separate branch and push it here as a draft PR as well and then we can pick and choose which variant to continue developing. possibly, that would be a good point to ask others for comments as well.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

@akohlmey OK, yes that seems like a good outline. I discussed with @tadmor and we converged to the following:

  • create a kim_style (name is negotiable) with init and defn (again feel free to suggest better, or more "LAMMPS-ish", names for these keywords).

  • The kim_style defn ex_model_name species1 species2 ... command would be used for both Models and Simulator Models. For regular KIM models it would just execute appropriate pair_style kim and pair_coeff * commands. For Simulator Models, it would first verify that the model-init (units, atom_style, etc.) commands are consistent with the currently defined settings (if not, exit with error), and then execute the model-defn commands. These commands would all be similar to pair_style in that they must/may be executed in the input after a simulation is setup via a read_data, read_restart, or create_box command.

  • The kim_style init ex_model_name form would be optional. If it is used, it should simply execute the model-init commands for a Simulator Model and do nothing for a regular KIM model. These commands would all be similar to atom_style in that they must/may be executed in the input before a simulation is setup via a read_data, read_restart, or create_box command.

Thoughts?

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

@akohlmey I've added three additional example SMs to the openkim/kim-api@025cf85 branch.

I'm still working on getting an example that uses things like bond_style, angle_style, etc.

Update kim_init to use KIM::Collections::GetItemType()
A bit of a cleaner solution.  Should be no visible change for users.
@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 26, 2019

@akohlmey Thanks for the comments. I'll work on an update to the examples.

while i am at it. it would probably also be worth considering to put some kind of version identifier into $HOME/.kim-api/config so that it is detected when one tries to access/load models compiled with a different (and possibly incompatible) version of the KIM API. i had such a folder, but from an older compile/install, and then there were errors managing models as some of the stored config settings were wrong.

Thanks for the report. Do you have any additional details? They would be helpful for me to design a solution.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Thanks for the report. Do you have any additional details? They would be helpful for me to design a solution.

not anymore, and i don't remember the details. i found the hint for what was going wrong in the kim.log file. i had to wipe out everything for testing the PR and - at that time - didn't think about making a backup for debugging purposes.

what i would suggest to do is something like this: when compiling/installing the kim-api, generate a hash that is unique for that specific installation (e.g. based on KIM API version, os/compiler, and date/time) and then use this hash as a folder inside of ~/.kim-api so that multiple installations could be used concurrently, each maintaining their own collection of SMs and PMs. if you also record some info about when/how/by-whom this was compiled, you could have a little tool showing you an inventory of what is available and where the installation is located that matches each hash. you can even do some cleaning up, if there are orphaned folders etc.
of course, this is primarily useful for people like me, that only does compiling, testing, and programming, and less so for others. but at the very least, this hash could be added as an entry to ~/.kim-api/config and then print a warning, when trying to manage a collection generated with a different installation. all depends on what you would consider compatible and what not. on linux things are a bit complex, because there are several compilers, which complicate things in terms of what is compatible with what.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 29, 2019

@akkamesh I'm adding some logging to the kim_query command and I'm getting this error

/Users/relliott/unison-sync/KIM/git/lammps/src/KIM/kim_query.cpp: In member function 'void LAMMPS_NS::KimQuery::echo_var_assign(const string&, const string&) const':
/Users/relliott/unison-sync/KIM/git/lammps/src/KIM/kim_query.cpp:309:29: error: 'int LAMMPS_NS::Input::echo_screen' is protected within this context
  309 |     if ((screen) && (input->echo_screen)) fputs(mesg.c_str(),screen);
      |                             ^~~~~~~~~~~
In file included from /Users/relliott/unison-sync/KIM/git/lammps/src/KIM/kim_query.cpp:64:
/Users/relliott/unison-sync/KIM/git/lammps/src/input.h:47:7: note: declared protected here
   47 |   int echo_screen;             // 0 = no, 1 = yes
      |       ^~~~~~~~~~~

I can't figure out why. It seem to be nearly identical to what is done in kim_init.cpp which works fine. Any quick ideas?

ellio167 and others added some commits Jul 29, 2019

add write_echo() method to Input class for logging implicit commands …
…where the echo command would send explicit ones
@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

I'm adding some logging to the kim_query command and I'm getting this error

/Users/relliott/unison-sync/KIM/git/lammps/src/KIM/kim_query.cpp: In member function 'void LAMMPS_NS::KimQuery::echo_var_assign(const string&, const string&) const':
/Users/relliott/unison-sync/KIM/git/lammps/src/KIM/kim_query.cpp:309:29: error: 'int LAMMPS_NS::Input::echo_screen' is protected within this context
  309 |     if ((screen) && (input->echo_screen)) fputs(mesg.c_str(),screen);
      |                             ^~~~~~~~~~~
In file included from /Users/relliott/unison-sync/KIM/git/lammps/src/KIM/kim_query.cpp:64:
/Users/relliott/unison-sync/KIM/git/lammps/src/input.h:47:7: note: declared protected here
   47 |   int echo_screen;             // 0 = no, 1 = yes
      |       ^~~~~~~~~~~

I can't figure out why. It seem to be nearly identical to what is done in kim_init.cpp which works fine. Any quick ideas?

you should have added KimQuery as a friend class to Input in input.h. But since this is becoming too messy (it was ok for just one class), I just added an Input::write_echo() API that is now used throughout and thus also got rid of the requirement for friend class access to data.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 29, 2019

@akohlmey Cool. Thanks.

make kim_query log of variable setting look more like other KIM log m…
…essages

this adds BEGIN/END marker comments and echoes the actual command line
that the kim_query command implicitly executes
@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@ellio167 made one more change. this is now better aligned with the other command logs by having a BEGIN/END marker and echoing the equivalent LAMMPS input that is implicitly executed.

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 29, 2019

@akohlmey Thanks. Actually, there is an option where multiple variables are set. I'll adjust what you have, so that the BEGIN/END markers are not repeated...

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@ellio167 BTW i am puzzled as to why the build-docs-pr check fails. i cannot reproduce it on multiple local machines. so i've asked @rbberger to look into what is different on our integration server and perhaps figure out a way to make this work. because the issues pointed out are false.

@rbberger

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

I'm on it

Merge branch 'master' into kim-simulator-models
# Resolved Conflicts:
#	src/KIM/kim_query.cpp
#	src/KIM/pair_kim.cpp
@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

merged in master (for the last time?) to resolve merge conflicts.

@rbberger

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

There are non-ASCII characters in those two files compute.txt and kim_commands.txt, which apparently leads to an error in Python 3.6 inside of the container.

akohlmey added some commits Jul 30, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@sjplimp @stanmoore1 this has now all known (to me) kinks straightened out. a year in the working...

@ellio167

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

@akohlmey Thanks for all the help. I think this will be great!

@akohlmey akohlmey merged commit 8fa4efa into lammps:master Jul 30, 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

@ellio167 ellio167 deleted the ellio167:kim-simulator-models branch Jul 30, 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.