Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow mapping of rings with different hybridization #30

Closed
hannahbaumann opened this issue Dec 21, 2023 · 10 comments
Closed

Disallow mapping of rings with different hybridization #30

hannahbaumann opened this issue Dec 21, 2023 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@hannahbaumann
Copy link
Contributor

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
Copy link
Contributor

@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

@richardjgowers richardjgowers self-assigned this Dec 21, 2023
@IAlibay
Copy link
Member

IAlibay commented Dec 21, 2023

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
Copy link
Contributor

RiesBen commented Jan 29, 2024

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
Copy link
Member

IAlibay commented Jan 30, 2024

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
Copy link
Contributor

RiesBen commented Jan 31, 2024

@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
Copy link
Member

IAlibay commented Jan 31, 2024

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
Copy link
Member

IAlibay commented Jan 31, 2024

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
Copy link
Contributor

RiesBen commented Jan 31, 2024

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
Copy link
Contributor

RiesBen commented Jan 31, 2024

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 RiesBen self-assigned this Feb 1, 2024
@RiesBen RiesBen added the enhancement New feature or request label Feb 1, 2024
@RiesBen
Copy link
Contributor

RiesBen commented Apr 17, 2024

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

@RiesBen RiesBen closed this as completed Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants