espresso icon indicating copy to clipboard operation
espresso copied to clipboard

Reaction methods bug fixes

Open jngrad opened this issue 3 years ago • 2 comments

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)

jngrad avatar Oct 07 '22 16:10 jngrad

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_part is 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 use steps as 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

pm-blanco avatar Oct 12 '22 14:10 pm-blanco

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.

jngrad avatar Oct 13 '22 11:10 jngrad