chrisiacovella
commented
5 months ago Related to this, @jchodera writes in the comments on the MC move refactor PR:
I don't like the idea of having a method like self._accept_or_reject() use some object fields (current and previous reduced potential energies) and some arguments (log proposal ratio, PRNG, and method---which is already stored as an object field) to perform the calculations. Especially thinking ahead to perhaps making the acceptance behavior a class, it would make more sense if we just forced everything to be an argument for a mathematics-based computation like the acceptance probability here.
Even better woudl be to have it return the log probability of acceptance, since we could store that during self._update_statistics() in addition to the decision. The statistics of log(Paccept) are valuable in figuring out how bad of an acceptance problem we have, and can even be used to estimate very small acceptance ratios.
Currently we have a parameter in the MC moves called "acceptance_criteria" which is just a string; this string is used to toggle different criteria (although currently only supporting metropolis-tastings) We had noted previously this would probably be good to have this be in a separate class to allow for modularity (and enable different routines without having to modify core routines).
This issue is a place holder to revamping this treatment, as we will need to define the API.