Merge pull request #290 from OpenBioSim/fix_287

Fix issue #287

Author: lohedges

corelib/src/libs/SireIO/sdf.cpp

@@ -235,11 +235,11 @@ namespace SireIO
                 case 0:
                     return 0;
                 case 1:
-                    return 1;
+                    return 3;
                 case 2:
                     return 2;
                 case 3:
-                    return 3;
+                    return 1;
                 case 4:
                     return 0;
                 case 5:

doc/source/changelog.rst

@@ -17,6 +17,8 @@ organisation on `GitHub <https://github.com/openbiosim/sire>`__.
 ------------------------------------------------------------------------------------------
 
 * Please add an item to this CHANGELOG for any new features or bug fixes when creating a PR.
+* Allow user to force fresh inference of stereochemistry when converting to RDKit format.
+* Fix setting of positive formal charge when reading SDF files.
 
 `2024.4.0 <https://github.com/openbiosim/sire/compare/2024.3.1...2024.4.0>`__ - Feb 2025
 ----------------------------------------------------------------------------------------

tests/convert/test_rdkit.py

@@ -118,7 +118,6 @@ def test_rdkit_returns_null():
     "rdkit" not in sr.convert.supported_formats(),
     reason="rdkit support is not available",
 )
-@pytest.mark.xfail(reason="SMILES now mismatches since SDF stereochemistry is preserved")
 def test_rdkit_infer_bonds(ejm55_sdf, ejm55_gro):
     sdf = ejm55_sdf[0].molecule()
     gro = ejm55_gro["not (protein or water)"].molecule()

tests/io/test_sdf.py

@@ -6,3 +6,38 @@ def test_cis_or_trans():
     Make sure that we can read an SDF file containing a cis or trans double bond.
     """
     mols = sr.load_test_files("cis_trans_double_bond.sdf")
+
+
+def test_charge():
+    """
+    Make sure that the SDF formal charge field is parsed correctly.
+    """
+    mol = sr.load_test_files("charge.sdf")[0]
+
+    from math import isclose
+
+    import tempfile
+
+    # SDF format has a weird mapping for formal charge. Here the keys are
+    # the SDF value, and the values are the formal charge.
+    mapping = {
+        7: -3,
+        6: -2,
+        5: -1,
+        0: 0,
+        3: 1,
+        2: 2,
+        1: 3,
+    }
+
+    # Make sure the charges are parsed correctly.
+    for c0, c1 in zip(mol.property("formal_charge").to_list(), mapping.values()):
+        assert isclose(c0.value(), c1)
+
+    # Write back to file.
+    with tempfile.NamedTemporaryFile(suffix=".sdf") as f:
+        sr.save(mol, f.name)
+
+        # Read back in and check that the charges are still correct.
+        for c0, c1 in zip(mol.property("formal_charge").to_list(), mapping.values()):
+            assert isclose(c0.value(), c1)

wrapper/Convert/SireRDKit/sire_rdkit.cpp

@@ -606,6 +606,12 @@ namespace SireRDKit
         SireMaths::Vector *coords_data = coords.data();
         bool has_coords = false;
 
+        bool force_stereo_inference = false;
+        if (map.specified("force_stereo_inference"))
+        {
+            force_stereo_inference = map["force_stereo_inference"].value().asABoolean();
+        }
+
         for (int i = 0; i < atoms.count(); ++i)
         {
             const auto atom = atoms(i);
@@ -640,6 +646,9 @@ namespace SireRDKit
 
             a->setAtomicNum(element.nProtons());
 
+            // don't automatically add hydrogens
+            a->setNoImplicit(true);
+
             elements.append(element);
 
             try
@@ -776,9 +785,11 @@ namespace SireRDKit
         molecule.commitBatchEdit();
         molecule.updatePropertyCache(false);
 
-        if (atoms.count() > 1 and (not has_bond_info))
+        if (atoms.count() > 1 and (not has_bond_info or force_stereo_inference))
+        {
             // we need to infer the bond information
             infer_bond_info(molecule);
+        }
 
         // try each sanitisation step in turn, skipping failed
         try
@@ -855,7 +866,7 @@ namespace SireRDKit
 
         // try assigning stereochemistry from 3D coordinates as it is the most
         // reliable way to do it
-        if (has_coords)
+        if (has_coords and not force_stereo_inference)
         {
             try
             {