OpenFreeEnergy / kartograf

This package contains tools for setting up hybrid-topology FE calculations
https://kartograf.readthedocs.io/
MIT License
25 stars 2 forks source link

Disallow mapping of rings with different hybridization #30

Closed hannahbaumann closed 7 months ago

hannahbaumann commented 11 months ago

When using the Kartograf atom mapper for the shp2 benchmarking system (https://github.com/OpenFreeEnergy/protein-ligand-benchmark), some mappings include the mapping of ring systems that have a different hybridization. Would it be possible to add a filter that disallows such mappings?

Screen Shot 2023-12-21 at 1 42 22 PM

richardjgowers commented 11 months ago

@hannahbaumann iirc this is something that is flagged in a paper somewhere as sometimes being good and sometimes bad right? Something about strict/not strict rules for ring mapping. So this would be a filter that people could choose to apply/not apply

IAlibay commented 11 months ago

I believe that not mapping these cases is the default for most other tooling. Probably the best way to look at this is what the worst case scenario would be: including them could lead to discontinues in the free energy cycle, what would not including then lead to? Slower convergence in cases where the ring is sufficiently constrained?

Unless there's a non sampling related case (i.e. actual alchemical problem), I would suggest taking the stance of accuracy over precision.

RiesBen commented 9 months ago

I kinda agree with all of you :D

I think we should implement such a filter in our tooling, so that if users choose to exclude such mappings, they can do that. I think it is a pretty frequent case, where depending on the simulation protocol you need those or don't.

Fun fact: we did not use this filter in the Kartograf paper and it did not kill us, but the torsion distribution was more noisy for a fully aromatic system, than it should be. I think there is no black-and-white answer if the bonded terms are correctly treated in this context.

IAlibay commented 9 months ago

I believe when @hannahbaumann and I spoke to John two weeks ago, the topic of ring hybridization changes did come up and he specifically made a comment that the HTF would not be able to handle it in a correct manner.

RiesBen commented 9 months ago

@IAlibay This might be True for the HTF we currently use from that source, but this is not necessarily true for all HT approaches (ignoring the sampling challenge). and also depends actually on the use case of your atom mapper. I would suggest we have a call on this if we want to discuss this in detail.

But nevertheless, the filter would be good to have.

IAlibay commented 9 months ago

I think our tooling should default to what our methods require, a user should be able to confidently use the default behaviour and know it'll "just work". At the end of the day kartograf is intended as a drop in replacement specifically for the OpenMM RFE protocol and that's not going to stop being our MVP any time soon.

IAlibay commented 9 months ago

Another approach to look at this is - what open source hybrid topology methods that currently exist guarantee that this would work? I'm not sure that I know one.

RiesBen commented 9 months ago

I think our tooling should default to what our methods require, a user should be able to confidently use the default behaviour and know it'll "just work". At the end of the day kartograf is intended as a drop in replacement specifically for the OpenMM RFE protocol and that's not going to stop being our MVP any time soon.

But this could also be realized by how you implement Kartograf in OpenFE, right? So like we do with Lomap we could have our customized settings there, and live a more general approach in the repo.

RiesBen commented 9 months ago

Another approach to look at this is - what open source hybrid topology methods that currently exist guarantee that this would work? I'm not sure that I know one.

one example, were bond types are perturbed: https://pubs.acs.org/doi/10.1021/acs.jcim.1c00428

RiesBen commented 7 months ago

This was merged, and I see in #41 we have our first use case example :) let's continue from there :)