Skip to content

Make MAX_GHOSTS a compile-time flag#314

Draft
janbridley wants to merge 3 commits into
trunkfrom
feat/configurable-max-ghosts
Draft

Make MAX_GHOSTS a compile-time flag#314
janbridley wants to merge 3 commits into
trunkfrom
feat/configurable-max-ghosts

Conversation

@janbridley

@janbridley janbridley commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Description

Using a build script and a const fn that reads the environment variable, the max number of ghosts (arrayvec size) is now configurable. This is the first I've heard of build scripts, so please let me know if anything seems incorrect.

HOOMD_MAX_GHOSTS=12 cargo test # current default, needed for 12-12
HOOMD_MAX_GHOSTS=8 cargo test # sufficient for 3d, and the 8-8 boundary

Motivation and context

Max ghosts for a cartesian space requires up to N^2-1 ghosts per particle, and in other space could require arbitrary counts. This PR makes the constant an environment variable, such that users can set the value as high as they need. This fixes things for users, but tests are still limited to the default setting right now (e.g. setting to 8 breaks the 12-12 boundary tests). We could fix this by configuring that env variable in CI, but I'm not sure the best way to do it for devs.

How has this been tested?

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.

@janbridley janbridley marked this pull request as ready for review June 12, 2026 16:25
@janbridley janbridley requested review from joaander and mthran June 12, 2026 16:25
@joaander

Copy link
Copy Markdown
Member

I'm not a fan of using an environment variable for this. That moves the setting out of band with the normal cargo build sequence. How should a user know that it needs to be set if they have a dependency of a dependency on hoomd-microstate?

We talked about two options to solve this while you were out last week:

  1. Add MAX_GHOSTS as yet another generic parameter of Microstate (and GenerateGhosts). This gives infinite flexibility at the cost of modifying all the lines of code that reference Microstate<B, S, X, C> and GenerateGhosts.
  2. Add feature flags like max_ghosts_16, max_ghosts_32, max_ghosts_64, .... This gives only discreet choices for the maximum number of ghosts, but the choice is made in cargo.toml and is therefore communicated properly up the dependency chain. The implementation would set MAX_GHOSTS to the largest value in all set feature flags.

With regards to unit tests: Option 1 allows each individual test to set MAX_GHOSTS appropriately and there is no problem. For Option 2, we would need to be careful to not introduce a large max_ghosts by default in the dependency chain. I haven't checked, but I wonder if it is possible to set separate feature flags in dependencies and dev-dependencies.

If you want to try option 1, please branch off md. I do not want to have to merge those changes with the huge number of changes already on md.

I suppose it might be possible to rewrite Microstate to need MAX_GHOSTS only in the impl block and not in the struct definition. I am unsure if we could maintain the same performance level while doing so.

@janbridley

Copy link
Copy Markdown
Contributor Author

I'm not a fan of using an environment variable for this. That moves the setting out of band with the normal cargo build sequence. How should a user know that it needs to be set if they have a dependency of a dependency on hoomd-microstate?

We talked about two options to solve this while you were out last week:

  1. Add MAX_GHOSTS as yet another generic parameter of Microstate (and GenerateGhosts). This gives infinite flexibility at the cost of modifying all the lines of code that reference Microstate<B, S, X, C> and GenerateGhosts.
  2. Add feature flags like max_ghosts_16, max_ghosts_32, max_ghosts_64, .... This gives only discreet choices for the maximum number of ghosts, but the choice is made in cargo.toml and is therefore communicated properly up the dependency chain. The implementation would set MAX_GHOSTS to the largest value in all set feature flags.

With regards to unit tests: Option 1 allows each individual test to set MAX_GHOSTS appropriately and there is no problem. For Option 2, we would need to be careful to not introduce a large max_ghosts by default in the dependency chain. I haven't checked, but I wonder if it is possible to set separate feature flags in dependencies and dev-dependencies.

If you want to try option 1, please branch off md. I do not want to have to merge those changes with the huge number of changes already on md.

I suppose it might be possible to rewrite Microstate to need MAX_GHOSTS only in the impl block and not in the struct definition. I am unsure if we could maintain the same performance level while doing so.

Thanks for the feedback, these seem like good options. I am leaning toward option 1, but will have to see what the diff looks like when I try it. I will base the branch of md as requested!

@janbridley janbridley marked this pull request as draft June 16, 2026 18:23
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