search

glotzerlab / hoomd-blue

Molecular dynamics and Monte Carlo soft matter simulation on GPUs.
http://glotzerlab.engin.umich.edu/hoomd-blue
BSD 3-Clause "New" or "Revised" License
324 stars 127 forks source link

Restrict `mpcd.collide.CellList` cell size #1813

Closed mphoward closed 1 month ago

mphoward commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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.