mosdef-hub / mbuild

A hierarchical, component based molecule builder
https://mbuild.mosdef.org
Other
173 stars 80 forks source link

Bad smiles unit test failing #1132

Closed chrisjonesBSU closed 1 year ago

chrisjonesBSU commented 1 year ago

Bug summary

In test_compound.py the unit test for passing in a bad smiles string to RDKit is failing by not raising an exception

2363     @pytest.mark.parametrize("bad_smiles", ["F[P-](F)(F)(F)(F)F"])
2364     @pytest.mark.skipif(not has_rdkit, reason="RDKit is not installed")
2365     def test_incorrect_rdkit_smiles(self, bad_smiles):
2366         with pytest.raises(
2367             MBuildError,
2368             match=r"RDKit was unable to generate " r"3D coordinates",
2369         ):
2370             mb.load(bad_smiles, smiles=True, backend="rdkit", seed=29)

I'm not getting any errors from mbuild or RDkit when I try to load that smiles string.

>>> import mbuild as mb
>>> comp = mb.load("F[P-](F)(F)(F)(F)F", smiles=True, backend="rdkit")
[12:36:05] UFFTYPER: Warning: hybridization set to SP3 for atom 1
[12:36:05] UFFTYPER: Unrecognized charge state for atom: 1
[12:36:05] UFFTYPER: Warning: hybridization set to SP3 for atom 1
[12:36:05] UFFTYPER: Unrecognized charge state for atom: 1
>>> 

I'm not familiar with what we are testing for here. Did this smiles string used to raise an error in RDKit?

daico007 commented 1 year ago

Yes, RDKit used to have checks for if the SMILES string for a few conditions (charge neutral, bond partners, etc.) and would fail. The SMILES string above is for the charge issue, but seems like they have some update to also cover those cases. We can turn that tests off for now, but I don't think it's anything detrimental.

chrisjonesBSU commented 1 year ago

Ok cool. So, keep it in the test suite, but skip it for now, or should we remove this test? I can add this into #1131

daico007 commented 1 year ago

I think we can remove it, and yes it can be included in #1131