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

Remove option to remove one bond or decorator #87

Open
bannanc opened this issue May 6, 2019 · 2 comments
Open

Remove option to remove one bond or decorator #87

bannanc opened this issue May 6, 2019 · 2 comments

Comments

@bannanc
Copy link
Member

bannanc commented May 6, 2019

While writing up the Reducer options for removing decorators it occurred to me that removing one or decorator at a time will never work on bonds.

For bonds, or'd together decorators in ChemPer are only the order of the bonds used to make the SMIRKS patterns. So removing one of those orders will never result in keeping clustering, where removing all of them could be ok.

For example, there are a fair number of SMIRKS in SMIRNOFF99Frosst which use double or aromatic bonds (=,:). If we need to keep a SMIRKS that matches both then trying to remove just the double decorator switching the bond to just aromatic doesn't make any sense.
However, we can imagine a situation where the order of the bond isn't actually important. In that case we could remove all of the decorators and switch the bond to and (~).

@bannanc
Copy link
Member Author

bannanc commented May 6, 2019

For reference this is here in the smirksifier.py file.

Assuming my logic on this gets verified I'll add it to PR #82.

@bannanc
Copy link
Member Author

bannanc commented Jul 2, 2019

I think this logic is sound, and I talked through it with Vickie at some point, but I'm not going to do it in #82 because I want version 1.0.0 to have the removal options I described in our paper. I'll do this as a separate PR after that release.

@bannanc
Copy link
Member Author

bannanc commented Dec 13, 2019

I never did this, so its up to whoever takes over

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant