3D MCS

[robbason]

I'm interested in adding the functionality to constrain atom matches by 3D distance in MCS. I saw Greg's gist giving an example of how to do this:
https://gist.github.com/greglandrum/365b0b5f6cfedfe79b7686c1c261deb3
This approach runs MCS and then removes any parts of the matches that correspond to atoms that are too far apart,
but I think there are some limitations to this approach. For example, let's say you have two molecules with a fixed core, and sticking off the core is a phenyl with a methyl at the 2 position, but one of the molecules has the methyl flipped 180 degrees. This approach will generate an MCS including the phenyl and the methyl substituent. Matches on that MCS will associate phenyl atoms to the atoms on the geometrically opposite side of the ring, and the geometric criterion will then remove them from the match, which is not really what you want.

A good use case for this would be in setting up maps for FEP, when you might have a docked pose for input ligands.

I've created a branch 'mcs3d' in my fork with a straightforward implementation of this. Since I haven't contributed to this project before, I don't want to just bull ahead with a pull request!

Does this seem like a reasonable feature to add to MCS?

[greglandrum]

FWIW in case anyone else wants to take a look at this, here's an easy link to see what @robbason has changed: master...robbason:mcs3d

I think this is a good idea and completely makes sense to integrate at the C++ level as you are suggesting.
One thing I don't think makes sense in the current implementation: it loops over all the conformations of the two molecules and just requires that the atoms be close to each other in some conformation pair. I'd modify that to just always use the default conformation of each molecule and then document that this is how it works.

[bp-kelley]

How often is the atom compare called? It might make sense to pay the loops for this upfront and save the matched pairs. I agree with the default conformation, however it might make sense to add the ensemble option that loops over all confs. ---- Brian Kelley

…

On Jan 1, 2021, at 4:45 AM, Greg Landrum ***@***.***> wrote:  FWIW in case anyone else wants to take a look at this, here's an easy link to see what @robbason has changed: master...robbason:mcs3d I think this is a good idea and completely makes sense to integrate at the C++ level as you are suggesting. One thing I don't think makes sense in the current implementation: it loops over all the conformations of the two molecules and just requires that the atoms be close to each other in some conformation pair. I'd modify that to just always use the default conformation of each molecule and then document that this is how it works. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[robbason]

Thanks for the feedback. I think there's a reasonable scenario in which you might want to put in multiple docking poses. In that case the output should probably tell you which mcs matched which conformers. This gets big combinatorially very quickly! I'll add an option for this but have the default use the first conformer. I'll explore caching the distances too - makes sense. Process wise should I move this to an issue? And what's the best way of making sure this builds on all platforms? Is that best sorted out after a pull request? Thanks again!

…

On Fri, Jan 1, 2021, 10:03 AM Brian Kelley ***@***.***> wrote: How often is the atom compare called? It might make sense to pay the loops for this upfront and save the matched pairs. I agree with the default conformation, however it might make sense to add the ensemble option that loops over all confs. ---- Brian Kelley > On Jan 1, 2021, at 4:45 AM, Greg Landrum ***@***.***> wrote: > >  > FWIW in case anyone else wants to take a look at this, here's an easy link to see what @robbason has changed: master...robbason:mcs3d > > I think this is a good idea and completely makes sense to integrate at the C++ level as you are suggesting. > One thing I don't think makes sense in the current implementation: it loops over all the conformations of the two molecules and just requires that the atoms be close to each other in some conformation pair. I'd modify that to just always use the default conformation of each molecule and then document that this is how it works. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub, or unsubscribe. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3675 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHMXSRUPXKWWLVP6X4O5UDSXXP3JANCNFSM4VOF2CNA> .

[greglandrum]

I think there's a reasonable scenario in which you
might want to put in multiple docking poses.

Agreed, it seems like if you are doing that then you want to find the 3D MCSs between m_a-conf1 and m_b-conf1, m_a-conf2 and m_b-conf1, m_a-conf1 and m_b-conf2 (etc) and then return the MCS + pair of conformation IDs which lead to the largest MCS, right? That's different from what your code is currently doing.

Process wise should I move this to an issue? And what's the best way of
making sure this builds on all platforms? Is that best sorted out after a
pull request?

The easiest way I know of is to do the PR, mark it as a draft, and then look to see what happens with the CI builds. Once you're happy that things are passing you can remove the "draft" flag from the PR and we can do the full review

[robbason]

Yes, I agree that the logic you saw in my branch should be changed. I think it makes the most sense to let the caller pass in a list of the conformer IDs they want (or default to first conformers, of course) and just give the MCS for that list of conformers, since they may have their own logic for which conformation they want to use and may not just want the one with the largest MCS.

Doing this further work is highlighting the need to work with the userData pointer passed into the atom and bond compare functions. At the risk of overreaching, I'm doing some cleanup here, as the way userData is used doesn't seem to be in the spirit of the userData pattern, which means I'll be tweaking the code @ptosco wrote adding support for Python FMCS functors in #3023. I'm creating a container that can hold the RingMatchTableSet used in the current MaximumCommonSubgraph code as well as the data for the Python FMCS functors. It's not obvious to me whether we should keep a userData pointer as well as it doesn't seem to be used as such. This should be clearer once I create a PR and I can always change it then.

[bp-kelley]

It might be worth looking at this old PR

FMCS Atom Distance Cutoff
#509

That was abandoned for reasons I'm not to clear on now.

[robbason]

Ah thanks! I didn't know about that one. I'll check in with Yutong.