Open IAlibay opened 1 year ago
@IAlibay Thanks! We would love to have this, please feel free to make a PR once you have the fix.
@ijpulidos - our approach I think is going to be to deal with ThreeParticleAverageSite for now and disallow other virtual sites (since it's the only case we encounter in the wild today). Then we'll add in extra bits for OpenFF as they need it, would that work for y'all here too?
Expected behaviour
Passing a system with virtual sites in the non-core, non-unique portions of the old & new systems, should lead to those virtual sites existing in the hybrid system. For example a system that has been parameterised with
amber/tip4pew_standard.xml
(with ThreeParticleAverageSite virtual sites) should work.Actual behaviour
The code in HybridTopologyFactory._handle_virtual_sites throws the following exception when handling a system solvated with tip4pew:
Further context
Original OpenFE issue: https://github.com/OpenFreeEnergy/openfe/issues/394 We're fixing this in OpenFE's version of HybridTopologyFactory here: https://github.com/OpenFreeEnergy/openfe/pull/395 I'm happy to push it back into perses' HTF once that's done.
My understanding is that there are two issues here:
I think the answer here is that we'll have to create methods for each VirtualSite subclass that we want to handle that correctly copies the virtual site with the right hybrid system indices for the reference particles. For now I've got it working for ThreeParticleAverageSite, but I don't have a test case with other types of virtual sites (I think OpenFF wants at least ThreeParticleAverageSite and LocalCoordinateSites).