Skip to content

Conversation

@wkdarko
Copy link
Contributor

@wkdarko wkdarko commented Sep 13, 2025

Description

This PR implements the Lees-Edwards boundary conditions for performing shear flow simulations.

Motivation and context

The implementation will allow users to simulate bulk rheology under full periodic boundary conditions.

How has this been tested?

Unit tests on box deformation methods were added.

Checklist:

  • I have reviewed the Contributor Guidelines.
  • I agree with the terms of the HOOMD-blue Contributor Agreement.
  • My name is on the list of contributors (sphinx-doc/credits.rst) in the pull request source branch.
  • I have summarized these changes in CHANGELOG.rst following the established format.

@github-actions
Copy link

github-actions bot commented Oct 3, 2025

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale There has been no activity on this for some time. label Oct 3, 2025
@mphoward mphoward removed the stale There has been no activity on this for some time. label Oct 3, 2025
@joaander
Copy link
Member

joaander commented Oct 3, 2025

Now that 5.4 is out, this is next on my list to review. I'll look at the proposed changes soon.

@mphoward
Copy link
Collaborator

mphoward commented Oct 3, 2025

Thanks! I’ve also asked Kwabena to merge up trunk-minor to keep this current.

Copy link
Collaborator

@mphoward mphoward left a comment

Choose a reason for hiding this comment

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

Thanks @wkdarko! This looks good to me based on our internal rounds of review. I had a couple small requests below related to the fixes you pushed to the communicators, then I think it should be good to go from my side.

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks! The code is clean and the changes are minimal.

I am concerned about the large number of minImage, wrap, and shift methods. Some return results, others modify references in place. To someone reading this code a year from now, it will not be clear what method is called. Could you add a suffix to the new methods to ameliorate this problem?

This PR is also missing a user interface to set the deformation rate and documentation of the feature in features.rst. We need to clearly document what classes in HOOMD-blue interoperate with the deformation rate and what ones do not. At this point, it is not clear to me whether that can be done in features.rst or if it needs to be mentioned in each class's documentation. Ideally, classes that don't support box deformations will raise an error when the deformation rate is non-zero.

Longer term (after this PR), you should add a tutorial to https://github.com/glotzerlab/hoomd-examples that shows users how to perform Lees-Edwards simulations.

@mphoward
Copy link
Collaborator

I am concerned about the large number of minImage, wrap, and shift methods. Some return results, others modify references in place. To someone reading this code a year from now, it will not be clear what method is called. Could you add a suffix to the new methods to ameliorate this problem?

The methods we added have the same argument / return types as the existing methods, so we thought overloading to include a velocity argument would be cleaner than appending a suffix. Would it be OK if we address this by expanding the doxygen documentation for the new methods? We can explain that they are used in the same way as the position-only methods but when the velocity also needs to be manipulated. Otherwise, we will need to go back through all the files in the diff manually to rename.

This PR is also missing a user interface to set the deformation rate and documentation of the feature in features.rst. We need to clearly document what classes in HOOMD-blue interoperate with the deformation rate and what ones do not. At this point, it is not clear to me whether that can be done in features.rst or if it needs to be mentioned in each class's documentation. Ideally, classes that don't support box deformations will raise an error when the deformation rate is non-zero.

Yes, we are working on the user interface to do this in a separate PR! We were worried this one was getting too bulky. If you are OK with it, I think this PR could be merged as a temporarily undocumented feature because it doesn't change any behavior of existing code without an interface to set the deformation rates. (They are all zero by default.)

@wkdarko left a comment on #2120 to ask about how it would be best to implement the deformation, and we would appreciate your feedback there before we proceed too far!

Because of how we have modified the BoxDim, all the MD integration methods and pair forces should work with the deformation. I'm not sure if there are any other MD features that would break... the thermo compute will return unphysical values, but that is expected for NEMD. I know that some things in MPCD will break (and we will fix them), so we will error out and document that in the follow up PR until we have them fixed. We plan to do some thorough validation and comparison with LAMMPS once we have the user interface created.

Longer term (after this PR), you should add a tutorial to https://github.com/glotzerlab/hoomd-examples that shows users how to perform Lees-Edwards simulations.

Yes, definitely will do!

@joaander
Copy link
Member

Yes, we are working on the user interface to do this in a separate PR! We were worried this one was getting too bulky. If you are OK with it, I think this PR could be merged as a temporarily undocumented feature because it doesn't change any behavior of existing code without an interface to set the deformation rates. (They are all zero by default.)

Yes, I would prefer everything in one PR. Reviewing the changes in batches is helpful.

@wkdarko left a comment on #2120 to ask about how it would be best to implement the deformation, and we would appreciate your feedback there before we proceed too far!

Understood, I will move this discussion there.

@mphoward mphoward added md MD component mpcd MPCD component labels Nov 11, 2025
@joaander joaander deleted the branch glotzerlab:trunk November 21, 2025 22:03
@joaander joaander closed this Nov 21, 2025
@mphoward
Copy link
Collaborator

@wkdarko you'll need to update your fork and reopen this PR onto trunk per #2183.

@joaander joaander reopened this Dec 1, 2025
@joaander joaander changed the base branch from trunk-minor to trunk December 1, 2025 13:17
@joaander
Copy link
Member

joaander commented Dec 1, 2025

@wkdarko you'll need to update your fork and reopen this PR onto trunk per #2183.

I recreated trunk-minor temporarily so that I could reopen this PR and change the base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

md MD component mpcd MPCD component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants