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

Updates to dynamical_matrix #1359

Merged
merged 5 commits into from Mar 21, 2019

Conversation

Projects
None yet
4 participants
@martok
Copy link
Collaborator

martok commented Mar 6, 2019

Summary

Collected updates and fixes to the USER-PHONON dynamical_matrix command. See below for detailed list of changes.
This is still a work-in-progress, creating the PR now for visibility only.

Related Issues

This updates PR #1314.

Author(s)

Sebastian Hütter (OvGU)

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

Bugfixes to a brand new command. There will be changes, but not many users should be affected.

Implementation Notes

  • Check if an atom->map exists before running (will not have one ie. for atom_style atomic)
  • Fix writing binary dynmat files (also streamline writeMatrix method for readability)
  • Output progress indicator similar to USER-DIFFRACTION

ToDo

  • Correct generation of groupmaps
  • Dynmat of groups other than all (shape, order, MPI)
  • Early-out optimization if an atom will not generate a row or column q

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted

  • 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
  • A package specific README file has been included or updated
  • One or more example input decks are included
@martok

This comment has been minimized.

Copy link
Collaborator Author

martok commented Mar 7, 2019

This last commit changed the groupmap and some issues with MPI comm using the wrong element size/count. Now everything that is a global atom id should be a bigint. With this, I think I changed what the groupmap means (new definition: see comment in create_groupmap). In any case, it didn't work before for groups!=all.
Now you can use a subgroup and get the correct matrix dimension. For some forcefields (basically all that have neighbor cutoffs), this even gives quite correct results for not-periodic subgroups.

@martok martok force-pushed the martok:dynamical-matrix branch from b7d1bfa to 0f857f0 Mar 13, 2019

@martok martok force-pushed the martok:dynamical-matrix branch from 0f857f0 to 7fe1cce Mar 13, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Mar 20, 2019

@martok @charlessievers any updates on the improvements to the dynamical matrix command? should we merge the bugfixes in the PR now (and have further improvements in another PR)?

@martok

This comment has been minimized.

Copy link
Collaborator Author

martok commented Mar 20, 2019

"Works for me".
What was decided regarding unit systems? Did we want to change something or leave the hard conversion factors for now?
Should I redo the example outputs? There are some differences, but they are in numerical noise (equal to 8 significants).

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Mar 20, 2019

"Works for me".

What was decided regarding unit systems? Did we want to change something or leave the hard conversion factors for now?

if i remember correctly, this primarily needs to be better documented (units of input and output and keyword to convert/not convert).

Should I redo the example outputs? There are some differences, but they are in numerical noise (equal to 8 significants).

not worth doing now. we do it, when all changes, including the 3rd derivatives command are in.

@martok martok marked this pull request as ready for review Mar 20, 2019

@akohlmey akohlmey requested a review from sjplimp Mar 20, 2019

@martok

This comment has been minimized.

Copy link
Collaborator Author

martok commented Mar 20, 2019

Alright, then I'm calling it feature-ready. For review, it'd be good if you could verify I got all bigints right.

@akohlmey akohlmey assigned akohlmey and unassigned martok Mar 20, 2019

@athomps
Copy link
Contributor

athomps left a comment

I am okay with releasing this as is. However, there are problems with units:

  1. No information is provided on doc page
  2. The units are hard-coded to be 10 J/mol, regardless of unit style requested by user
  3. This is inconsistent with fix_phonon command:
    "The calculated dynamical matrix elements are written out in energy/distance^2/mass units. "
@martok

This comment has been minimized.

Copy link
Collaborator Author

martok commented Mar 20, 2019

Style regular gives results in the current energy/distance/mass without conversion. Only eskm is in 10J/mol. I've come to like it, as:

The square root of the eigenvalues divided by 2pi give you THz with said units

But I'd be all for renaming that to something that makes this clear, as well as document it. There was also a python example for plotting PhDOS from the results using ASE, but that got discarded when the third_order part was separated out.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Mar 21, 2019

But I'd be all for renaming that to something that makes this clear, as well as document it. There was also a python example for plotting PhDOS from the results using ASE, but that got discarded when the third_order part was separated out.

I hope, that @charlessievers will put these examples back in some form, but properly documented and with explanations for how to run LAMMPS through ASE (which would be a good addition to the manual in any case). At least, that was the original plan. As far as I read the python code, ASE was only used to set up the system, with a strategic use of a write_data command, those could likely be easily transformed into regular LAMMPS inputs. The post-processing seemed to be independent from using ASE, and thus could be used with either ASE based or plain LAMMPS input and thus it would be desirable to have that code (using NumPp/SciPy for the linear algebra work) as a standalone example.

@akohlmey akohlmey merged commit 76b9c00 into lammps:master Mar 21, 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
@charlessievers

This comment has been minimized.

Copy link
Collaborator

charlessievers commented Mar 27, 2019

I am glad you like the ESKM style @martok!

I would prefer the style to be called ESKM, but that is for selfish reasons. If usability is a concern, we can change the style name to something else.

The Install.sh script needs to be changed until I finish fixing the third order calculator, which I am not sure when I will get to.

@akohlmey I would love to add my python scripts into the repo if you are all interested. There is a decent amount of documentation on ASE's end as to how to run lammps calculations.
ASE documentation
Do you have a little more direction for what type of documentation you would like?

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Mar 27, 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.