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

Walker member Multiplicity should be an integer value #5059

Open
PDoakORNL opened this issue Jun 25, 2024 · 2 comments
Open

Walker member Multiplicity should be an integer value #5059

PDoakORNL opened this issue Jun 25, 2024 · 2 comments

Comments

@PDoakORNL
Copy link
Contributor

The semantics of Multiplicity as used in the population control of QMCPACK are such that it can only have a non negative integer value. It's currently a floating point value that is very frequently cast to int. There are several possible reason I can think of, in the past someone thought type safety wasn't useful, as origninally conceived of it could have been float, without any documented proof left behind it was a performance issue someone was trying to save defining an MPI type or having two appropriately typed MPI buffers. At anyrate it should have a type that reflects its semantics.

Describe the solution you'd like
It should be a signed integer type. Additions and subtractions are done to multiplicity, so it should not be an unsigned type. Unsigned types do not enforce positive values they were designed to handle address spaces and later repurposed as "sizes" but there main feature with C++ math operators is to "wrap around" on over and underflow this not a sensible behavior for Multiplicity.

Describe alternatives you've considered
Do nothing...

Additional context
Add any other context or screenshots about the feature request here.

@prckent
Copy link
Contributor

prckent commented Jun 26, 2024

I agree with this. However, besides use in population control, unfortunately multiplicity appears to have been hijacked in CSVMC for other purposes, so there is some disentangling to be done.

For the purposes of walker control, my understanding is that the use of the multiplicity is only to say which walkers are going to be multiplied (i.e. multiplicity=2,3,4...) or killed (multiplicity=0), while satisfying the various minimums/maximums/rate limits. i.e. The multiplicity is a rather short lived property which is only used during the walker population control. Therefore it could be an integer and technically doesn't even need to be kept as long term state.

@ye-luo
Copy link
Contributor

ye-luo commented Jun 26, 2024

Agreed. multiplicity should be short lived during population control.

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

No branches or pull requests

3 participants