Switch to OpenBabel 3 #2088
Switch to OpenBabel 3 #2088
Conversation
|
Running tests locally I get only two problems now This is the same InChI that I had to mark as a work-in-progress because OpenBabel cannot parse it into a valid Molecule. the test is def test_nitrogenated_birad(self):
smiles = '[CH]=C[N]'
mol = Molecule().from_smiles(smiles)
paths = find_allyl_delocalization_paths(mol.atoms[3])
self.assertTrue(paths)after reading from_smiles the molecule is compared to the RMG website which gives so I think it's just an atom ordering issue. I'll see if there's an alternative SMILES that passes both before and after. |
|
The unit tests now pass for me locally, on Mac OS X, where according to |
|
See #756 for the conversion to the official CCLib. |
|
Current test failures in the travis log all to do with parsing mopac files with cclib: |
rwest
force-pushed
the
openbabel3
branch
2 times, most recently
from
9147fe2 to
459de4e
Compare
Codecov Report
@@ Coverage Diff @@
## master #2088 +/- ##
==========================================
+ Coverage 47.58% 47.61% +0.03%
==========================================
Files 89 89
Lines 23575 23575
Branches 6133 6133
==========================================
+ Hits 11217 11226 +9
+ Misses 11174 11166 -8
+ Partials 1184 1183 -1
Continue to review full report at Codecov.
|
|
Looks good to me as well. I guess rmg-tests fail due to known reasons? |
|
I get a wrong Molecule object for phenanthrene ( |
|
Oh :-( http://openbabel.org/docs/current/Features/Radicals.html SMILES extensions for radicals Finishes with:
The URL says it's "current" documentation but the HTML says 2.3.1 Guess one could post to the openbabel user group mailing list Update: found the documentation for newer version https://open-babel.readthedocs.io/en/latest/Features/Radicals.html#smiles-extensions-for-radicals |
|
It might be our conversion from OBMol to Molecule. Or it might be OpenBabel's parsing of the SMILES. |
|
I get the correct Molecule, though, when I use the full SMILES |
|
I requested "posting permission" for the open babel forum. I got the same problem with |
|
Citing from @rwest's link: This extension is not as robust or as carefully considered as standard SMILES and should be used with restraint. A structure that uses c as a radical centre close to aromatic carbons can be confusing to read, and Open Babel’s SMILES parser can also be confused So I guess it's OK if it's "designed/known", although the previous version supported these cases better. We might want to document this though? |
|
Do we want to wait and not include this in the release? |
|
Does Not sure what the solution is. Issue a warning when using lower case letters in a SMILES that things may not be what you expect? Add a note in documentation? |
|
What were you expecting for |
|
For
for I think this is not an ordinary "bug", but rather perhaps a "design choice" by OB's devs. I recommend documenting it. We could make RMG crush if any |
|
Are you sure One way of thinking of the lower case letters is they essentially remove an H atom, so if C would have been CH2 (eg. in So if So I think, after digging through these, the new OpenBabel branch is behaving as I would expect it to, and you were testing it with some weird malformed SMILES. Is that unfair? Should we add code to spit out warnings in case others do the same? Either way, I think we go ahead with the change to OpenBabel3
Note: this notebook currently only tests SMILES string to OpenBabel, not all the way through to RMG Molecule objects |
|
You are absolutely correct. Thanks for getting to the bottom of this, I agree with the interpretation that NOW OB behaves "as expected". I'm still uncomfortable with the interpretation of the (weird) |
|
Alright lets get this in. Can you rebase? |
The release notes say it's an improvement https://github.com/openbabel/openbabel/releases/tag/openbabel-3-0-0 but it does break some things in the API https://open-babel.readthedocs.io/en/latest/UseTheLibrary/migration.html#migrating-to-3-0 so subsequent commits should fix RMG to be compatible.
The initial pull request in openbabel openbabel/openbabel#1576 sounds like it just broke radicals, to be fixed later. A follow-up openbabel/openbabel#1626 somewhat fixed things but "Now reading undervalent atoms from SMILES does not set the spin (it did previously)", with a comment "I didn't add an 'op' to set the spin state of undervalent atoms. It could be done of course, but the syntax was sufficiently involved (how to specify which atoms?, what default atoms?) that I'd rather wait until someone actually has a use case". The code introduced there for how to *draw* radicals gives some clues as to how you could handle it though, using the expected valence. That is the approach I take here. The SpinMultiplicity on the OBAtom object is left incorrect or undefined, and we fix it when converting from_ob_mol to an RMG Molecule.
This test used to pass with OpenBabel < 3.0, but I think the inchi is invalid?
or at least not standard.
OpenBabel reports:
Problems/mismatches: Mobile-H( Hydrogens: Locations or number, Number; Charge(s): Do not match)
and cactus.nci.nih.gov converts it to InChI=1S/C6H7O4/c1-2-4-9-6(7)3-5-10-8/h2-3,8H,1,5H2/q+1
which at least doesn't make OpenBabel complain. However, both have a net charge
and cause RMG to crash. I'm not sure what the molecule was ever supposed to represent.
The default is 0 so I won't call it for efficiency's sake, but this will make sure we don't forget that it's now determined this way by OpenBabel 3
…passes I think this test is equivalent, but writing the SMILES this way both old OpenBabel and new OpenBabel agree that the N is mol.atoms[0] and so the test passes.
This test had bene using an invalid InChI, which broke when we upgraded OpenBabel. Replaced it with a valid molecule.
|
Rebased, and added a small note to the documentation. Out of curiosity, I extended the notebook to compare the RDKit back end. In many cases it does similar things, it ends up with similar isomers for the C14H6 case but with different electronic structure. And it indeed crashes for some of the non-aromatic cases. |
Motivation or Problem
The release notes say OpenBabel 3 a major improvement
https://github.com/openbabel/openbabel/releases/tag/openbabel-3-0-0
but it does break some things in the API
https://open-babel.readthedocs.io/en/latest/UseTheLibrary/migration.html#migrating-to-3-0
so subsequent commits on this pull request should fix RMG to be compatible.
This pull request is currently a stub. Please feel free to contribute commits!
Description of Changes
Testing
Reviewer Tips
Further detailed commentary is in the individual commit messages.