search

QMCPACK / qmcpack

Main repository for QMCPACK, an open-source production level many-body ab initio Quantum Monte Carlo code for computing the electronic structure of atoms, molecules, and solids with full performance portable GPU support
http://www.qmcpack.org
Other
295 stars 138 forks source link

Walker member Multiplicity should be an integer value #5059

Open PDoakORNL opened 3 months ago

PDoakORNL commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Agreed. multiplicity should be short lived during population control.