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

Compute momentum #1529

Merged
merged 9 commits into from Jul 30, 2019

Conversation

@rupertnash
Copy link
Contributor

commented Jun 20, 2019

Summary

Add a compute for the total momentum of a group on atoms, in the style of compute KE

For now, I have added this to the main LAMMPS src/ directory but you may prefer that I contribute this to USER-MISC. If this is the case, please let me know and I will alter the PR to make it so.

This compute is useful for a larger suite of fixes (for coupling between LAMMPS an other codes using MUI https://github.com/MxUI/MUI) I plan to contribute back as a user package in the coming months.

Related Issues

Author(s)

Rupert Nash, EPCC, The University of Edinburgh, r.nash@epcc.ed.ac.uk

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

No

Implementation Notes

I cribbed the class definition from compute_ke.h/.cpp, changing from scalar to vector

This was tested on a variety of input scripts by also dumping atom velocities masses.

No other features are affected.

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted. Delete lines that don't apply

  • 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

Further Information, Files, and Links

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Thanks for putting in the effort to submit a pull request. But why not simply define the following atom style variables:

variable mx atom mass*vx
variable my atom mass*vy
variable mz atom mass*vz

and then access v_mx, v_my, and v_mz instead of using this compute

compute c1 all momentum

with access through c_c1[1], c_c1[2], and c_c1[3]?
Is there any use case, where using this compute offers significant advantages the atom style variables?

@rupertnash

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Thanks for looking at this! However I think you missed that this compute will calc the total momentum of all the atoms in a group whereas (as I understand it and I'm new to LAMMPS...) an atom style variable is per-atom. Is that right?

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Thanks for looking at this! However I think you missed that this compute will calc the total momentum of all the atoms in a group whereas (as I understand it and I'm new to LAMMPS...) an atom style variable is per-atom. Is that right?

you are correct, I missed that. however, that would just be one more line of input.

variable mx atom mass*vx
variable my atom mass*vy
variable mz atom mass*vz
compute c1 all reduce sum v_mx v_my v_mz
@rupertnash

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

OK! Thanks for the comments Axel. I guess this is a risk with LAMMPS being so big.

However since the code now exists, and would save users some effort/learning, I'm still willing to tick off the remaining items above, if you think it's of value to the users. If you think on balance it's better to use the variable + reduce compute in script then I'm happy if you just close the PR.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

OK! Thanks for the comments Axel. I guess this is a risk with LAMMPS being so big.

However since the code now exists, and would save users some effort/learning, I'm still willing to tick off the remaining items above, if you think it's of value to the users. If you think on balance it's better to use the variable + reduce compute in script then I'm happy if you just close the PR.

I am coming out neutral on this one. On one hand, this command allows to do with one line of input script, what otherwise requires four, on the other hand, it is additional code (albeit simple code) to maintain. I was asking for a particular use case for this command versus input script to tip the scales toward including it. If added, it should definitely go into the USER-MISC package, though.

I'll assign the PR to @sjplimp for a second opinion.

src/Makefile.list Outdated Show resolved Hide resolved
@sjplimp

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

I think it's fine to add to USER-MISC as a new compute.

@sjplimp
Copy link
Contributor

left a comment

just move to USER-MISC, and state that in command doc page,
also edit the USER-MISC/README

@akohlmey akohlmey added the needs_work label Jun 25, 2019

@akohlmey akohlmey added this to the Stable Release Summer 2019 milestone Jun 25, 2019

rupertnash added some commits Jul 1, 2019

@rupertnash

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

In the last few commits I've moved the compute to USER-MISC, updated the docs, and added a test input script. I think this is now ready to go.

@sjplimp

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

agreed: ready to go when you make the doc page edits. Thanks

@rupertnash

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Sorry @sjplimp - I don't follow which doc page needs updated. I've done

  • doc/src/compute.txt
  • doc/src/Commands_compute.txt
  • doc/src/compute_momentum.txt (note about USER-MISC in restrictions similar to e.g. compute_entropy_atom)
  • doc/src/computes.txt
  • src/USER-MISC/README
    Let me know what I've missed!
    Cheers
@sjplimp

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I just meant the paragraph at the bottom of compute_momentum.txt that I flagged
in an earlier comment. It has some wrong grammar.

@akohlmey akohlmey removed the needs_work label Jul 9, 2019

rupertnash added some commits Jul 22, 2019

@rupertnash

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

OK - I think I've got the grammar error- afraid I didn't see anything explicitly flagged. Also resolved the merge conflicts. Thanks.

@akohlmey akohlmey requested review from sjplimp and akohlmey Jul 29, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@sjplimp this also seems to be ready and waiting to be approved and assigned to me


[Output info:]

This compute calculates a global vector (the summed momentum) on

This comment has been minimized.

Copy link
@sjplimp

sjplimp Jul 29, 2019

Contributor

on -> of

doc/src/compute_momentum.txt Show resolved Hide resolved

@sjplimp sjplimp assigned akohlmey and unassigned sjplimp Jul 29, 2019

@akohlmey
Copy link
Member

left a comment

made some cosmetic changes

@akohlmey akohlmey merged commit 88523fd 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.