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

Added compute for Polyhedral Template Matching #1120

Merged
merged 8 commits into from Sep 28, 2018

Conversation

Projects
None yet
3 participants
@pmla
Copy link
Collaborator

pmla commented Sep 11, 2018

Purpose

This adds a compute for Polyhedral Template Matching (PTM), a method for analysis of local crystalline order.

Author(s)

Peter Mahler Larsen
Massachusetts Institute of Technology

Backward Compatibility

This compute should not affect backwards compatibility.

Implementation Notes

I have tested the new compute on a few data sets. The code for PTM itself is well tested now - it has been part of OVITO and ASAP for two years.

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

Example inputs are included in the documentation
The interface (compute_ptm_atom.cpp, compute_ptm_atom.h) follows the LAMMPS formatting guidelines. The rest of the source does not. It is a user package though.

Further Information, Files, and Links

The PTM paper can be found here:
https://doi.org/10.1088/0965-0393/24/5/055007

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 12, 2018

@pmla thanks for your contribution.
Due to the way how LAMMPS is including packages into the (conventional) build system, there is a bit of a problem with the naming of the files that are not the compute style class. Generic names like cell.{cpp,h} or quat.{cpp,h} and especially config.h should be reserved for the base functionality. Please either rename them into something that has a consistent identifying prefix (e.g. ptm_cell.cpp ptm_config.h ..., see USER-MEAMC as an example) or convert them into a library and put them into lib/ptm (see POEMS as an example).

@akohlmey akohlmey requested a review from sjplimp Sep 14, 2018

@pmla

This comment has been minimized.

Copy link
Collaborator

pmla commented Sep 20, 2018

@akohlmey Thanks for reviewing my pull request. I have made the changes you propose.

pmla

@akohlmey akohlmey self-assigned this Sep 20, 2018

@akohlmey akohlmey added the needs_work label Sep 20, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 20, 2018

@pmla thanks for taking the time. this takes care of the most obvious concern, and so i can take a closer look. now i am looking at function name and avoiding namespace clashes. there are two more issues from simply looking over the changes in the pull request.

  • the code uses (modified) functions from the Voro++ library. LAMMPS uses the (unmodified) Voro++ library in the VORONOI package. to avoid clashes and possible errors, i would suggest to place all of those into a renamed namespace, e.g. ptm_voro instead of voro.
  • while most utility functions are declared static (yay!), a few of those that are not have rather trivial names. and even if their header files are only included in .cpp files (yay! again), there could be name clashes at link time (boo!). thus it would be preferable to either rename those functions, e.g. to have them pre-fixed with a suitable string (e.g. ptm_ similar to the file names) or simply put them into a custom namespace (e.g. ptm or PTM) and then import that namespace where needed (the effect on symbol names would be equivalent and thus link-time conflicts would become much more unlikely).

sorry, if i am being a bit nitpicky here, but as the LAMMPS is continually growing with useful contribution like yours, we want to make sure, that the probability of conflicts with current or future contributions remains very low.

pmla
@pmla

This comment has been minimized.

Copy link
Collaborator

pmla commented Sep 20, 2018

@akohlmey thanks for your comments. I have wrapped all the PTM code in a "ptm" namespace, except for exported symbols and function definitions. These are all prefixed with "ptm_" though, so there shouldn't be any collisions.

@rbberger rbberger self-requested a review Sep 21, 2018

@pmla pmla requested a review from junghans as a code owner Sep 27, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 27, 2018

@pmla this looks like it can be merged soon. We just need to wait for the approval of the build system related changes. i've updated the documentation and a few relevant files in the build and github support. Please consider providing at least one small and fully runnable example in examples/USER/ptm it will help people to get started.

@akohlmey akohlmey removed the needs_work label Sep 27, 2018

@akohlmey akohlmey merged commit 4fe23c3 into lammps:master Sep 28, 2018

5 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/kokkos_omp 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

@pmla pmla deleted the pmla:polyhedral-template-matching branch Sep 28, 2018

@pmla

This comment has been minimized.

Copy link
Collaborator

pmla commented Sep 28, 2018

Thanks for your help with getting this into LAMMPS @akohlmey
I will create an example in a separate pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment