[WIP] Add XC GHMC + RESPA integrators.

[kyleabeauchamp]

• [x] Implement feature / fix bug
• [x] Add tests
• [ ] Update documentation as needed
• [ ] Update changelog

@maxentile Here's how I preserved the commit history while giving you a writeable branch:

# add my fork
git remote add kab ...
git fetch kab
git checkout -b kab_thermostatted
git merge kab/thermostatted
git push origin kab_thermostatted

I also took a stab at fixing the merge conflicts in setup.py and init.py, but please review.

[maxentile]

Thanks so much!

[maxentile]

I noticed the body of GHMCRESPAIntegrator is commented out: is that waiting on something else or can it just be un-commented and tested?

[kyleabeauchamp]

Will check tonight

[maxentile]

Reviewed the GHMCIntegrator and XCGHMCIntegrator, and they both look great to me! When we decide whether to include the RESPA integrators, I'll take a closer look at those.

My only substantive comment so far is that I'd prefer not to include the effective_timestep property (or the derived effective_ns_per_day property) computed from acceptance rate, since sample autocorrelation isn't identical to the acceptance rate. (E.g. Figure 1 of https://arxiv.org/abs/1209.5944 -- autocorrelation times can be much larger than suggested by the acceptance rate, and also depend on the amount of partial momentum refreshment.) Is it okay if I remove these properties?

My remaining to-do's for tomorrow:

• [x] Add XCHMC-specific test: correct behavior as the number of extra chances and steps per extra chance are varied.
• [ ] Remove old GHMCIntegrator (currently a few-line subclass of LangevinIntegrator), replace with references to this new GHMCIntegrator, and make sure that won't break the API in any annoying way.

[kyleabeauchamp]

Removing those properties SGTM. I agree they are incomplete characterizations that lack the autocorrelation contribution, although I wonder if they could still be useful for rough, back-of-the-envelope performance tuning. In particular, it's nice to have a performance estimate that doesn't require tracking an observable and doing time series analysis.

[kyleabeauchamp]

So my experimental warning is triggered multiple times along the subclass hierarchy, which is a minor nit but possibly annoying during tests.

[kyleabeauchamp]

Regarding removal of old GHMCIntegrator: is there any reason to keep that as a "Reference Implementation" for testing? Not sure.

[maxentile]

Removing those properties SGTM. I agree they are incomplete characterizations that lack the autocorrelation contribution, although I wonder if they could still be useful for rough, back-of-the-envelope performance tuning. In particular, it's nice to have a performance estimate that doesn't require tracking an observable and doing time series analysis.

Okay! I'll remove them for now -- they're easy enough for users to compute from the other available properties in cases where that's the appropriate target for performance tuning.

So my experimental warning is triggered multiple times along the subclass hierarchy, which is a minor nit but possibly annoying during tests.

I think that's fine!

Regarding removal of old GHMCIntegrator: is there any reason to keep that as a "Reference Implementation" for testing? Not sure.

Hmm, I'm not sure if there's a convenient way to test the two implementations for equivalent (pathwise) behavior -- I think it's better just use the checks that the GHMCIntegrator gives known correct results for tractable testsystems. Could maybe check with @jchodera , @pgrinaway , @andrrizzi ?

[jchodera]

Remove old GHMCIntegrator (currently a few-line subclass of LangevinIntegrator), replace with references to this new GHMCIntegrator, and make sure that won't break the API in any annoying way.

That sounds great to me!

Thanks so much for putting this together!