Reaction methods bug fixes
Description of changes:
- track particles with default-constructed type 0 (fixes #4588)
- restore the original particle velocity when a Monte Carlo displacement move is rejected (fixes #4587)
Thanks for involving me in this PR! There are some things that I am not convinced about the MC displacement move method:
- I think that the input variable
n_partis ambiguous: one could propose a new configuration moving n_part (as in the current implentation) or one could choose n_part particles and perform an MC trial move with each. For consistency with the rest of MC methods, I propose that instead we usestepsas input variable, standing for the number of MC steps that the method would do. Then each MC step one particle is randomly chosen and moved, and the move is accepted or rejected according the probability given by the canonical ensemble. - Instead of having a method for moving only particles of one specific type, I think that the method should by default try to move any particle. Then, one could provide an optional argument (something like
list_particle_types_to_move) which could be used to restrict which particles are selected.
@davidbbeyer do you think that such an implementation would still be compatible for your project? If yes and all of you agree with this alternative implementation I volunteer to change the code accordingly
Offline discussion with @pm-blanco: the Monte Carlo displacement move method in its current state could benefit from a few improvements, but that work might take a couple of weeks to complete. There is also a newly discovered bug when combining constant-pH with Widom insertion, which may be relevant for Monte Carlo displacement moves too, since displacement moves can be carried out from any reaction method class instance.
Since this PR fixes important bugs and is blocking the parallel Monte Carlo project, we agreed that this PR should be merged now. The other points can be taken care of in an upcoming PR that will be orthogonal to the parallel MC project.