Skip to content

Revised velocity verlet scheme to prevent unphysical sticking in DEM simulations.#4351

Merged
akohlmey merged 29 commits into
lammps:developfrom
dhairyaiitb:develop
Mar 5, 2025
Merged

Revised velocity verlet scheme to prevent unphysical sticking in DEM simulations.#4351
akohlmey merged 29 commits into
lammps:developfrom
dhairyaiitb:develop

Conversation

@dhairyaiitb

@dhairyaiitb dhairyaiitb commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Summary

The half-step lag in the standard velocity-Verlet can lead to errors in DEM simulations. These errors accumulate when we calculate the integral history term using velocity components which have been calculated using velocities from half-step and positions from full step. The revised velocity-Verlet approach resolves this issue.

Related Issue(s)

Author(s)

Dhairya R. Vyas, Postdoctoral researcher, Department of Mechanical Engineering, Northwestern University

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

Implementation Notes

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 conventional build system
  • The feature has been verified to work with the CMake based build system
  • Suitable tests have been added to the unittest tree.
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

@akohlmey

akohlmey commented Oct 7, 2024

Copy link
Copy Markdown
Member

@dhairyaiitb please go to the src folder, type make fix-whitespace and commit the resulting changes.

@akohlmey akohlmey requested a review from dsbolin October 9, 2024 14:40
@jtclemm

jtclemm commented Oct 21, 2024

Copy link
Copy Markdown
Collaborator

As discussed, a few changes are needed:

  1. an option to turn on/off the new integration scheme and preserve the default behavior (with documentation)
  2. rescalings ideally should be moved into general methods to simplify future additions of new contact models
  3. if there's a minimal input script that demonstrates the impact of these changes, it would be very helpful to include as an example

Comment thread src/GRANULAR/granular_model.cpp Outdated
@dhairyaiitb

Copy link
Copy Markdown
Contributor Author

I have cleaned up the comments and added a test example. I will add documentation and possibly another example in the coming days.

@jtclemm jtclemm marked this pull request as draft November 20, 2024 00:47
@dhairyaiitb

Copy link
Copy Markdown
Contributor Author

Wishing you a very Happy New Year, Joel!

I have updated the documentation with examples illustrating how to use this modification. Additionally, I have some good news: we have received reviews from the journal Computer Physics Communications. The reviewers suggested a few minor changes, which are related to presentation rather than the algorithm. I have also cited this paper into the documentation. Please note that this version of documentation is not for publishing online but specifically for you to review the documentation. I will update the details once the paper is accepted and published online, which will hopefully happen soon.

@akohlmey

Copy link
Copy Markdown
Member

@dhairyaiitb your pull request has a conflict with upstream. You need to check out the current develop branch and merge it into your feature branch and resolve the merge conflict in the process and push it back.

Please also note that your branch has whitespace issues again(!).

@dhairyaiitb dhairyaiitb requested a review from jtclemm January 10, 2025 01:28
@dhairyaiitb dhairyaiitb marked this pull request as ready for review January 10, 2025 01:47
@jtclemm

jtclemm commented Jan 16, 2025

Copy link
Copy Markdown
Collaborator

Thanks for the update, glad to hear you got good reviews. I'll try and find some time to review soon, hopefully next week.

Comment thread src/GRANULAR/gran_sub_mod.cpp Outdated
Comment thread src/GRANULAR/gran_sub_mod_rolling.cpp Outdated
Comment thread src/GRANULAR/gran_sub_mod_tangential.cpp Outdated
Comment thread src/GRANULAR/granular_model.cpp Outdated
@jtclemm

jtclemm commented Feb 19, 2025

Copy link
Copy Markdown
Collaborator

Thanks, those changes cover all my major comments. There are a few minor stylistic changes (e.g. spacing and variable names) I want to make. Then, if those look good with you, I'll get Dan to review.

@dhairyaiitb

Copy link
Copy Markdown
Contributor Author

Thanks Joel.

@jtclemm

jtclemm commented Feb 19, 2025

Copy link
Copy Markdown
Collaborator

Hey Dhairya, can you check the changes I just made? Primarily, I swapped the example so synchronization is the default (and added a wall so particles don't get lost) and renaming nx->nx_unrotated and nxuse->nx so the default name is more intuitive. Thanks!

I'll ping Dan to review.

Comment thread doc/src/pair_granular.rst

@jtclemm jtclemm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking comment until Dan can review

@dhairyaiitb

Copy link
Copy Markdown
Contributor Author

Hey Dhairya, can you check the changes I just made? Primarily, I swapped the example so synchronization is the default (and added a wall so particles don't get lost) and renaming nx->nx_unrotated and nxuse->nx so the default name is more intuitive. Thanks!

I'll ping Dan to review.

Looks great. Thanks Joel.

@dsbolin

dsbolin commented Mar 4, 2025

Copy link
Copy Markdown
Collaborator

Dhairya,
Sorry for the delay. I had a look through the code and the paper. While I wish there was a way to implement it without having modifications in so many places, I can't think of a way to do it, and this seems to work. More importantly, I think this will be a nice way to bring attention to issues with velocity Verlet for history/velocity-dependent pairwise interactions. Looks good to me!

@dhairyaiitb

Copy link
Copy Markdown
Contributor Author

Hi Dan,

Thank you for reviewing the code and paper. Glad it looks good to you. We were able to implement it thanks to your and Joel’s help. I really appreciate both of your guidance and support!

@akohlmey akohlmey moved this from Inactive to Ready for Merge in LAMMPS Pull Requests Mar 5, 2025
@akohlmey akohlmey merged commit 6da1c62 into lammps:develop Mar 5, 2025
@github-project-automation github-project-automation Bot moved this from Ready for Merge to Done in LAMMPS Pull Requests Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

4 participants