Skip to content

Spherical cell list#313

Open
mthran wants to merge 18 commits into
trunkfrom
spherical_cell_list
Open

Spherical cell list#313
mthran wants to merge 18 commits into
trunkfrom
spherical_cell_list

Conversation

@mthran

@mthran mthran commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Description

This PR introduces methods and traits necessary to implement cell lists and ParallelSweep for Spherical simulations:

  • A new struct SphericalVecCell is implemented whose structure is almost identical to the already existing VecCell. Most of the methods and functionality are identical because Spherical<N> is embedded in N-dimensional Euclidean space. The search radii for SphericalVecCell may be specified either using the Spherical geodesic metric or through the Euclidean metric in the embedding space.
  • Some structs and methods in vec_cell.rs which were previously private have been promoted to pub(crate) so as to avoid duplicate code in spherical_vec_cell.rs.
  • A new boundary ClosedSpherical<N> which implements Cover<Spherical<N>> is introduced. This is distinct from the pre-existing Closed<T> boundary because the latter implements methods such as Scale and Volume which are irrelevant and potentially misleading in Spherical<N> simulations.
image

Motivation and context

This PR significantly improves the speed of Spherical<N> simulations, which previously were too slow to run production simulations.

How has this been tested?

spherical_vec_cell.rs contains unit tests for the methods which are unique to itself. I have also run simulations for Spherical<3> and Spherical<4> using the new methods and verified that they are consistent with older simulations using AllPairs.

Checklist:

  • I have reviewed the Contributor Guidelines.
  • I agree with the terms of the hoomd-rs Contributor Agreement.
  • My name is on the list of contributors (doc/src/credits.md) in the pull request source branch.
  • I have summarized these changes in release-notes.md following the established format.

@joaander joaander left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't completed a full review yet. Here is what I have so far.

I question whether ClosedSpherical is the right name. Technically, the boundary is open as it allows bodies to move over the entire space. With that established, the problematic implementation is the blanket implementation of GenerateGhosts<S> for Open. Open does not depend on the vector space, so there is no way to express the maximum interaction range correctly. Also, the Cover doesn't make sense on open boundaries that are unbounded.

I don't have a concrete suggestion to fix this right now. Microstate uses Open as a default, so we can't just make Open generic. I guess we could use a suitable synonym of Open so that we could replace ClosedSpherical<N> with OpenSynonym<Spherical<N>>. Somehow, that synonym should indicate that the boundary is both open in once sense and bounded in another... That type could then naturally be used to express e.g. cylindrical coordinate spaces as well as spherical ones.

I reduced code duplication in HypercuboidCheckerboard. I haven't had a chance to review spherical_vec_cell.rs in full, but there must be some way that we can avoid all the code duplication there. Either via the same method I used in HypercuboidCheckerboard or by implementing inheritance via composition (where SphericalVecCell holds a VecCell as a field and directs most method calls to it internally).

Comment thread hoomd-mc/src/hypercuboid.rs Outdated
impl<const N: usize> Checkerboard<Spherical<N>> for HypercuboidCheckerboard<N> {
#[inline]
fn point_to_space_index(&self, point: &Spherical<N>) -> Option<usize> {
<Self as Checkerboard<Cartesian<N>>>::point_to_space_index(self, &Cartesian::from(*point.coordinates()))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here I removed the code duplication by calling the existing method with the Cartesian coordinates of the spherical point.

@joaander

Copy link
Copy Markdown
Member

On second thought, my previous suggestions regarding ClosedSpherical do not work out. The best synonym I can think of is "Free" which I'm not thrilled about. Also, the boundary type should need no fields. Making it generic on a vector space would require a phantom data field which would be unnecessarily awkward.

I now think we should only rename ClosedSpherical to OpenSpherical to reflect that the bodies are free to move about the entire spherical vector space. That would prevent confusion in the unlikely eventuality that we add some type of closed boundary that limits bodies to only a portion of the sphere.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants