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

enable fix rigid commands to add gravity to COM of rigid bodies #1801

Merged
merged 8 commits into from Jan 8, 2020

Conversation

@sjplimp
Copy link
Contributor

sjplimp commented Dec 6, 2019

Summary

Fix gravity works by adding a gravity force per particle. This is incorrect for rigid bodies with overlapping particles, e.g. a dense packing of spheriods. It results in gravity being too strong. And if the rigid body is asymmetric, could result in gravity applying a torque to the particle. This PR adds
a "gravity fixID" option to the fix rigid commands, so that they can use the gravity vector produced by fix gravity and apply the force directly to the center of mass of the rigid body. A "disable" option is also added to the fix gravity command to turn off its forces if there are no non-rigid particles to add gravity to.

Related Issues

If this addresses an open GitHub issue for this project, please mention the issue number here, and describe the relation. Use the phrases fixes #221 or closes #135, when you want an issue to be automatically closed when the pull request is merged

Author(s)

Steve and Dan Bolintineanu (Sandia)

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

For rigid bodies with overlapping particles, this will change the effect of fix gravity, if used correctly.

Implementation Notes

Provide any relevant details about how the changes are implemented, how correctness was verified, how other features - if any - in LAMMPS 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

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

@sjplimp

This comment has been minimized.

Copy link
Contributor Author

sjplimp commented Dec 6, 2019

@dsbolin Please try out these changes and see if they work as you expect. If they do, I will update the doc pages, and we can then merge it.

@sjplimp

This comment has been minimized.

Copy link
Contributor Author

sjplimp commented Dec 19, 2019

@akohlmey I think this PR is now ready, except small chunks of code need to be added to the USER-OMP versions of fix rigid and rigid/small. In the compute_forces_and_torques methods.

@sjplimp

This comment has been minimized.

Copy link
Contributor Author

sjplimp commented Dec 19, 2019

Added documentation for the new options on fix gravity and fix rigid. Also noticed that the fix rigid infile keyword was called inpfile in the code, but still infile in the doc page. Not sure when that change occurred. Kept the variable name inpfile in the code, but changed the input script option to infile to agree with the doc page.

@sjplimp sjplimp requested a review from akohlmey as a code owner Dec 20, 2019
@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Dec 20, 2019

@sjplimp @dsbolin it would be nice, if we could add an example input to the LAMMPS distribution that uses the inpfile option and possibly also fix gravity so we have a way to check, if things work correctly and a demo of how this is supposed to work.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jan 6, 2020

@sjplimp @dsbolin please see my previous comment. with reference to it, would you prefer this PR is merged right away and the example added later, or would the example better be added later. My preference would be to have the demonstration included here, but I am willing to accommodate.

@sjplimp

This comment has been minimized.

Copy link
Contributor Author

sjplimp commented Jan 6, 2020

@akohlmey @dsbollin Since Dan has already tested this works, I don't think an example is necessary. But if he has a small one that runs quickly, we could add it to examples/granular or examples/pour. It would be a nice addition since overlapped bodies with the infile option are not trivial to setup.

@dsbolin

This comment has been minimized.

Copy link
Collaborator

dsbolin commented Jan 7, 2020

@sjplimp @akohlmey Sure thing, I have a short example here:
https://github.com/dsbolin/lammps/tree/rigid-gravity

Want me to make a separate pull request? Sorry, I couldn't figure out how to make a pull request into the 'rigid-gravity' PR/branch.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jan 7, 2020

@akohlmey @dsbollin Since Dan has already tested this works, I don't think an example is necessary.

@sjplimp
I am not concerned about the addition being correct and properly tested. What I am concerned about is, that we don't break this feature when we incorporate future modifications.

This is still one of the major deficiencies in our (limited) regression testing for LAMMPS, that we don't have a good coverage of available features and that we too often assume that changes have no (unintended) side effects (which they do have occasionally). Having tested examples to document the status quo when new features are added should become as normal as insisting on documentation.
Sadly, we don't have a well structured and easy to add to repository for that, but that is on the TODO list.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jan 7, 2020

@sjplimp @akohlmey Sure thing, I have a short example here:
https://github.com/dsbolin/lammps/tree/rigid-gravity

Want me to make a separate pull request? Sorry, I couldn't figure out how to make a pull request into the 'rigid-gravity' PR/branch.

@dsbolin
to create a PR you first need to do a comparison across forks, this is done in general with an URL of this kind https://github.com/theiraccount/theirrepo/compare/theirbranch...myaccount:mybranch which translates in this case to https://github.com/lammps/lammps/compare/rigid-gravity...dsbolin:rigid-gravity. When going to this URL you can review the differences and a (green) button to submit a pull request should appear.
Of course submitting a separate PR is an acceptable, option, too, but it would be cleaner on my side to have this combined. So please give the first option a try and let me know, if you need further assistance.

@dsbolin

This comment has been minimized.

Copy link
Collaborator

dsbolin commented Jan 7, 2020

@akohlmey Thanks, just made the PR into the lammps/rigid-gravity branch, see #1823

Adds an example using new options in fix rigid and fix gravity
@akohlmey akohlmey merged commit c5768ac into master Jan 8, 2020
@akohlmey akohlmey deleted the rigid-gravity branch Jan 8, 2020
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.