-
Notifications
You must be signed in to change notification settings - Fork 229
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
Make surface molecules from smiles using Argon workaround #2607
base: main
Are you sure you want to change the base?
Make surface molecules from smiles using Argon workaround #2607
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Could we have a couple unit tests? I think there are some templates to copy. Can we do a round-trip (i.e. generate SMILES-like strings with X in, that reproduce the original molecule on import)?
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
2b17b3c
to
4732aa1
Compare
This is working with the test code described above, so now I just need to put that into an actual unit test and it'll be ready to go. |
Unit tests added, but don't work yet because the SMILES string from the rdkit translator's to_smiles() uses 'Pt' instead of 'X'. Need to figure out where is the most appropriate place to make this final translation, but then it should be ready |
Some mission creep: RMG molecule had no way to distinguish between specific Pt atom and more general surface X atom. I wanted So, I added a specific Pt atom type with the intention of being able to add other metals later on. It passes my simple test cases, but now I need to clean up the PR and look for other places this might cause problems for RMG-cat. |
Richard's working on a related PR: #2701. The rdkit * might be a more elegant way of doing this than the Ar workaround |
Motivation or Problem
The issue is described here, #2603, but basically RDKit complains when you try to make a surface molecule using smiles.
Description of Changes
This PR adds a workaround where you initialize the molecule using Ar as a replacement for X to get the adjacency list and then remove the extra pairs of electrons and change Ar back to X to get the originally intended molecule.
Testing
Run this code to test out instantiating all the species in the Surface family training reactions:
NOTE TO SELF -- DON'T FORGET TO REBUILD!