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

Restrict mpcd.collide.CellList cell size #1813

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

mphoward
Copy link
Collaborator

Description

We are actively working to relax the requirement that the MPCD cells be cubic (#773). Allowing the user to specify, change, or access the scalar cell_size is problematic. To avoid introducing API breaking changes after MPCD is rereleased, I would prefer to specify the cell size is fixed at 1.0, which is the standard choice for most MPCD models anyway. We will then extend the CellList API to allow specifying the number of cells along each lattice vector in future, which will allow the cell size to change.

Motivation and context

This will help smoothen the release of MPCD.

How has this been tested?

Existing tests pass.

Change log

No change needed.

Checklist:

@mphoward mphoward requested review from a team as code owners June 12, 2024 18:03
@mphoward mphoward requested review from tcmoore3, tommy-waltmann and syjlee and removed request for a team June 12, 2024 18:03
@mphoward mphoward added the mpcd MPCD component label Jun 12, 2024
Copy link
Contributor

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

Why not just allow the user to specify how many cells there should be in each direction right now, instead of waiting for a future PR?

@mphoward
Copy link
Collaborator Author

Not a great answer, but because this was a quick fix and I’m leaving for a week after today 😅 I have the code to do that on:

https://github.com/glotzerlab/hoomd-blue/tree/feature/mpcd-triclinic-box

I could try to move only the minimal required parts here and leave a comment on its status at the end of the day.

There would still need to be an error check in place that after the user specifies the number of cells (as a tuple) that the same cell size is calculated for all dimensions.

There would also no longer be a default cell list because we can’t guess a default number of cells the way we guess the default cell size. All the examples and tests would need to be updated to reflect that.

FWIW, users rarely change the cell size from 1. I’ve seen it done only once with HOOMD.

@tommy-waltmann
Copy link
Contributor

Okay, this seems like a special situation. It may be okay long as the changes on this PR are going to be overwritten quickly by a later PR.

Another thing I will point out is that this is an API-breaking change based on the trunk-minor branch, which is not normally something we would allow.

I think @joaander should have the final say on whether this type of PR is okay to merge.

@joaander
Copy link
Member

@tommy-waltmann It is not API breaking because the MPCD code is not yet in a tagged release version. @mphoward is proposing the quick fix in now so that the future PR is not API breaking.

@joaander joaander added the validate Execute long running validation tests on pull requests label Jun 13, 2024
@joaander joaander merged commit 260aa32 into trunk-minor Jun 13, 2024
96 of 97 checks passed
@joaander joaander deleted the fix/mpcd-cell-size branch June 13, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mpcd MPCD component validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants