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

Add compute_pressure_cylinder to USER-MISC package #1131

Merged
merged 16 commits into from Oct 9, 2018

Conversation

@ckadding
Copy link
Contributor

ckadding commented Sep 20, 2018

Added compute_pressure_cylinder files and documentation. This compute calculates the three orthogonal components of the pressure tensor in cylindrical coordinates.

Written by Cody K. Addington, North Carolina State University.

ckadding added some commits Sep 20, 2018

See the README file in the top-level LAMMPS directory.
------------------------------------------------------------------------- */

#include "math.h"

This comment has been minimized.

@akohlmey

akohlmey Sep 20, 2018

Member

please include "system headers", i.e those not provided by LAMMPS using angular brackets and also using C++ syntax (where supported by C++98). Example: #include "math.h" should become #include <cmath>

"units"_units.html. The number density values will be in
inverse volume "units"_units.html.

[Restrictions:] none

This comment has been minimized.

@akohlmey

akohlmey Oct 1, 2018

Member

Please add under restrictions, that this compute computes the contributions from pair styles (i.e. non-bonded interactions) only and that it requires functionality not available by most manybody pair styles. see compute group/group. There also is the issue of how pair styles for use with a kspace style are handled. it looks to me, that those are not compatible to this as well. Finally, it seems that this compute, that this does not consider kinetic contribution (by default) unlike other pressure compute styles in LAMMPS. so it is probably advisable to document this as well.

This comment has been minimized.

@ckadding

ckadding Oct 2, 2018

Contributor

I've clarified the restrictions and output in the documentation now, so hopefully everything is explicit enough now. Thanks!

ckadding added some commits Oct 2, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 2, 2018

@ckadding please note, that your contribution currently doesn't even compile. possibly a typo around the citeme string or you're missing a header.

@ckadding

This comment has been minimized.

Copy link
Contributor

ckadding commented Oct 2, 2018

@akohlmey Yep, sorry I am new to git. I am working between my local machine and the cloud, so will have the final version up soon!

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 2, 2018

@ckadding no worries. you are not alone, and we understand that it takes time to get used to everything. that is why i am mentioning these "technical" issues only now, after you have been successful in updating the other, more straightforward to understand and explain issues.

ckadding added some commits Oct 2, 2018

@akohlmey akohlmey requested review from sjplimp and stanmoore1 Oct 2, 2018

@akohlmey akohlmey added the enhancement label Oct 2, 2018

@akohlmey akohlmey self-assigned this Oct 2, 2018

@akohlmey akohlmey added this to the Stable Release Fall 2018 milestone Oct 2, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 2, 2018

@ckadding thanks for the changes, this is starting to look nice. i have added some changes to integrate your contribution properly into the manual and the build system.

i have one larger and one smaller nitpick:

  • we try to be consistent throughout LAMMPS with "natural constants". so please include the math_const.h header, use the MathConst name space, and replace all references to 3.14158 with MY_PI

  • it would be appreciated (but is not required) if the source code format would be closer to the LAMMPS style. 2 character indentation seems to be in use, so that is good. but we usually prefer to put the opening brace for if and for on the same line, not the next line, put a space after the ; in the for expressions, have parenthesis around multiple "chained" conditions to make precedence more obvious, and put a blank around comparison operators). the benefit for the LAMMPS maintainers, as it minimizes the changes in diffs, in case we edit files and then (re-)apply the indentation automatically from the programming editor. if there are many whitespace changes, it is difficult to tell, which is the relevant change.

thanks in advance,
axel.

akohlmey and others added some commits Oct 2, 2018

@ckadding

This comment has been minimized.

Copy link
Contributor

ckadding commented Oct 2, 2018

@akohlmey I think I have incorporated the changes you asked for, so please let me know if there is anything else I can do!

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 2, 2018

@ckadding thank you so much. i've done a few more cosmetic changes and this looks ready to me. Now we'll have to wait for an approving review from one of the other LAMMPS developers for having it merged.

@sjplimp

sjplimp approved these changes Oct 9, 2018

Copy link
Contributor

sjplimp left a comment

nice addition to the code

@akohlmey akohlmey merged commit 7910245 into lammps:master Oct 9, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment