Rascal atom and bond equivalences - #7612
Conversation
|
I don't think the failure is down to these changes. |
|
We're working on the osx builds, the clang toolchain was updated and it found some issues. Fixes are here |
|
@bp-kelley anything I can do to help? I probably have a bit of time tomorrow. |
|
If should be all fixed if you pull from master |
| // Set the atomic number of the atoms that match the SMARTS in | ||
| // RascalOptions.EquivalentAtoms to 110, 111 etc. These will | ||
| // be mapped back at the end. | ||
| void assignEquivalentAtoms(ROMol &mol, const std::string equivalentAtoms) { |
There was a problem hiding this comment.
const std::string &equivalences will prevent a potential copy
There was a problem hiding this comment.
Facepalm! Thanks.
DavidACosgrove
changed the title
| std::vector<std::string> classSmarts; | ||
| boost::split(classSmarts, equivalentAtoms, boost::is_any_of(" "), | ||
| boost::token_compress_on); | ||
| if (classSmarts.size() > 9) { |
There was a problem hiding this comment.
This seems like an arbitrary restriction, won't the code work with more than 9?
There was a problem hiding this comment.
As it says in the opening comment, that depends on what RDKit thinks of atomic numbers above 118. I assumed it would object though I didn’t check. Atoms that match a SMARTS class are all given the same atomic number starting at 110- it was a quick and dirty implementation. It could start lower than 110 but I was being exceptionally paranoid about what might be fed into it.
There was a problem hiding this comment.
I think it probably would still work with larger values, but (famous last words) "it's unlikely to ever really be a problem"
There was a problem hiding this comment.
Yes, I can’t think of a case where you’d want to have more than 9 types of equivalent atom. But, as you imply, you can never trust a chemist!
* C++ implementation of equivalent atoms via SMARTS. * Python wrapper. * Tidy. * Check for too many classes. * Better handling of spaces in split. * Pass string by reference. Doh. * Add ignoreBondOrders option.
* C++ implementation of equivalent atoms via SMARTS. * Python wrapper. * Tidy. * Check for too many classes. * Better handling of spaces in split. * Pass string by reference. Doh. * Add ignoreBondOrders option.
3 participants
Reference Issue
No issue, but referred to in #7565
What does this implement/fix? Explain your changes.
This PR allows the user to use SMARTS strings to create classes of equivalent atoms. For example, if you want all halogens to match each other, or allow aromatic carbon and nitrogen atoms to match each other. In that case the option string would be
[F,Cl,Br,I] [c,n]and
c1ccccc1Brandc1ncccc1Fwould have an MCES of 7 atoms. The SMARTS string of the match includes the SMARTS for the classes as well.The classes are implemented by giving all matching atoms for a SMARTS the same atomic number, starting at 110. There is thus an effective limit of 9 classes allowed (the atomic number can only go up to 118). A ValueErrorException is thrown if this occurs. Also, it will clearly get confused if there are Darmstadtium atoms in the molecule, but I think we can live with that.
Any other comments?