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

Added third order, added documentation, took out extraneous lines, ad… #1690

Merged
merged 8 commits into from Oct 8, 2019

Conversation

@charlessievers
Copy link
Collaborator

charlessievers commented Sep 20, 2019

…ded documentation figures.

Summary

Removed unnecessary lines from dynamical_matrix.cpp
Added third order force constant tensor calculator, it also uses finite difference to calculate.
Updated documentation for dynamical_matrix to help users understand the output and options.
Added documentation for third_order.

Related Issues

No related issues.

Author(s)

Name: Charles Sievers
University: UC Davis
Department: Chemistry
Lab: Donadio Lab
Email: charliesievers@cox.net.

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

Should not break backward compatibility.

Implementation Notes

Third order tensors were compared against a long standing in house third order tensor calculator.

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 CMake based build system

Further Information, Files, and Links

charlie sievers added 2 commits Sep 20, 2019
…ded documentation figures.
charlie sievers charlie sievers
@akohlmey akohlmey self-assigned this Sep 23, 2019
akohlmey added 4 commits Sep 23, 2019
@charlessievers charlessievers requested a review from rbberger as a code owner Sep 23, 2019
@akohlmey akohlmey requested review from athomps and sjplimp Sep 23, 2019
@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 23, 2019

@charlessievers i have updated the code base to the current master to avoid any problems with integration tests. i also renamed the "ballistico" keyword to be consistent with the "dynamical_matrix" command. it is difficult to justify those in the first place, but to have them named differently in both commands is just confusing to people. i leave it to you to decide, if you want to reflect the change in the implementation as well (not needed, but would be cleaner).

- indentation at 2 blanks
- use BIGINT_FORMAT consistently
- use MathSpecial::square() instead of pow(x,2)
@charlessievers

This comment has been minimized.

Copy link
Collaborator Author

charlessievers commented Sep 23, 2019

@charlessievers i have updated the code base to the current master to avoid any problems with integration tests. i also renamed the "ballistico" keyword to be consistent with the "dynamical_matrix" command. it is difficult to justify those in the first place, but to have them named differently in both commands is just confusing to people. i leave it to you to decide, if you want to reflect the change in the implementation as well (not needed, but would be cleaner).

Thank you @akohlmey for the edits. I completely understand, I will settle for eskm.

@charlessievers

This comment has been minimized.

Copy link
Collaborator Author

charlessievers commented Sep 23, 2019

@akohlmey would you be interested in me creating an option to access the dynamical matrix and third order tensor through the library interface? I understand that this would involve storing large slots of memory, but I think it would be useful for small to medium sized systems.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 25, 2019

@akohlmey would you be interested in me creating an option to access the dynamical matrix and third order tensor through the library interface? I understand that this would involve storing large slots of memory, but I think it would be useful for small to medium sized systems.

the tricky thing about this would be the question of what would allocate and free(!) the memory. since these are commands, their classes go out of scope after completion and thus there will be nothing that would know how to free the allocated memory. what might work could be to store the information to a (vector style) variable or a fix similar to fix store/state. that would make accessing it easy and the memory can be freed when the variable is deleted or the fix unfixed or the LAMMPS class instance deleted.

thus i think it might be sufficient to have example code that would read the generated file(s) into python and then post-process the data. if this file would be written to a ramdisk (e.g. /tmp on most linux machines), processing it could be rather fast.

@charlessievers

This comment has been minimized.

Copy link
Collaborator Author

charlessievers commented Sep 25, 2019

the tricky thing about this would be the question of what would allocate and free(!) the memory. since these are commands, their classes go out of scope after completion and thus there will be nothing that would know how to free the allocated memory. what might work could be to store the information to a (vector style) variable or a fix similar to fix store/state. that would make accessing it easy and the memory can be freed when the variable is deleted or the fix unfixed or the LAMMPS class instance deleted.

I see, that makes sense. I may look into storing the data in a variable for my next update.

@charlessievers

This comment has been minimized.

Copy link
Collaborator Author

charlessievers commented Sep 25, 2019

thus i think it might be sufficient to have example code that would read the generated file(s) into python and then post-process the data. if this file would be written to a ramdisk (e.g. /tmp on most linux machines), processing it could be rather fast.

I am not sure what you mean by example code. Are you referring to an example python scripts with ASE that I forgot to include in this PR?

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 25, 2019

thus i think it might be sufficient to have example code that would read the generated file(s) into python and then post-process the data. if this file would be written to a ramdisk (e.g. /tmp on most linux machines), processing it could be rather fast.

I am not sure what you mean by example code. Are you referring to an example python scripts with ASE that I forgot to include in this PR?

i don't mean the entire script, especially not the ASE part, but i vaguely recall that there was some post processing part that would read in the dynamical matrix from the file and do some numpy matrix operations on it.

@charlessievers

This comment has been minimized.

Copy link
Collaborator Author

charlessievers commented Sep 25, 2019

i don't mean the entire script, especially not the ASE part, but i vaguely recall that there was some post processing part that would read in the dynamical matrix from the file and do some numpy matrix operations on it.

I can add a script to read in the dynamical matrix and use it into this pull request (If you think it would enhance the PR).

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 25, 2019

i don't mean the entire script, especially not the ASE part, but i vaguely recall that there was some post processing part that would read in the dynamical matrix from the file and do some numpy matrix operations on it.

I can add a script to read in the dynamical matrix and use it into this pull request (If you think it would enhance the PR).

yes, i do.

Copy link
Contributor

sjplimp left a comment

looks good - thanks @charlessievers

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 8, 2019

looks good - thanks @charlessievers

@sjplimp does this mean you meant to approve this PR or that you want to wait for somebody else, e.g. @athomps to approve?

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Oct 8, 2019

I approve - @athomps Aidan may want to look at it as well

@athomps
athomps approved these changes Oct 8, 2019
Copy link
Contributor

athomps left a comment

I approve

@akohlmey akohlmey merged commit 0e10fbb into lammps:master Oct 8, 2019
8 checks passed
8 checks passed
lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/cmake/cmake-kokkos-cuda-pr head run ended
Details
lammps/pull-requests/cmake/cmake-kokkos-omp-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 charlessievers deleted the charlessievers:third_order branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.