Merge pull request #68 from xiaoruiDong/tests

Update testings

.coveragerc

@@ -0,0 +1,26 @@
+[run]
+branch = True
+source = rdmc
+omit =
+    rdmc/external/GeoMol/*
+    rdmc/external/rmg.py
+    rdmc/external/xyz2mol.py
+    rdmc/conformer_generation/*
+
+[report]
+show_missing = False
+exclude_lines =
+    def __repr__
+    logger.warning
+    if __name__ == .__main__.:
+omit =
+    rdmc/external/GeoMol/*
+    rdmc/external/rmg.py
+    rdmc/external/xyz2mol.py
+    rdmc/conformer_generation/*
+
+[html]
+directory = ./coverage/html
+
+[xml]
+output = ./coverage/coverage.xml

.github/workflows/ci.yaml

@@ -0,0 +1,80 @@
+name: Continuous Integration
+
+on:
+  schedule:
+    # * is a special character in YAML so you have to quote this string
+    - cron: "0 8 * * *"
+  push:
+  pull_request:
+  workflow_dispatch:
+
+concurrency:
+  group: actions-id-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  check-formatting:
+    name: Check Formatting Errors
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Install Dependencies
+        run: |
+          python -m pip install pycodestyle
+
+      - name: Run pycodestyle
+        run: |
+          pycodestyle --statistics \
+          --count \
+          --max-line-length 150 \
+          --max-doc-length 200 \
+          --ignore=E266,E501,W503,W505 \
+          --show-source \
+          .
+
+  build-and-test:
+    strategy:
+      fail-fast: false
+      matrix:
+        python-version: ["3.8", "3.9", "3.10", "3.11"]
+        os: [ubuntu-latest, windows-latest, macos-latest]
+
+    runs-on: ${{ matrix.os }}
+    defaults:
+      run:
+        shell: bash -l {0}
+    name: ${{ matrix.os }} Python ${{ matrix.python-version }} Subtest
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Setup Miniconda
+        uses: conda-incubator/setup-miniconda@v2
+        with:
+            activate-environment: rdmc_env
+            environment-file: environment.yml
+            miniforge-variant: mambaforge
+            miniforge-version: latest
+            python-version: ${{ matrix.python-version }}
+            auto-activate-base: true
+            use-mamba: true
+
+      - name: Install RDMC
+        run: python -m pip install --no-deps -vv ./
+
+      - name: Install Pytest
+        run: mamba install -y pytest pytest-cov pytest-check
+
+      - name: Mamba info
+        run: |
+          mamba info
+          mamba list
+
+      - name: Run Unit Tests
+        run: pytest
+
+      - name: Upload coverage reports to Codecov
+        uses: codecov/codecov-action@v3
+        if: ${{ matrix.os == 'ubuntu-latest' && matrix.python-version == '3.11' }}
+        env:
+          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
+          directory: ./coverage

pytest.ini

@@ -0,0 +1,16 @@
+[pytest]
+required_plugins =
+    pytest-cov
+    pytest-check
+filterwarnings =
+    ignore:.*escape seq.*:DeprecationWarning
+addopts =
+    --keep-duplicates
+    -vv
+    --cov=rdmc
+    --cov-config .coveragerc
+    --cov-report html
+    --cov-report xml
+    --cov-report term
+testpaths = test
+python_files = test_*.py

rdmc/conformer_generation/__init__.py

@@ -0,0 +1,2 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-

rdmc/conformer_generation/pruners.py

@@ -44,8 +44,8 @@ def prune_conformers(self,
         Prune conformers.
 
         Args:
-            current_mol_data (list[dict]): conformer data of the current iteration.
-            unique_mol_data (list[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
+            current_mol_data (List[dict]): conformer data of the current iteration.
+            unique_mol_data (List[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
             sort_by_energy (bool, optional): Whether to sort conformers by energy. Defaults to ``True``.
             return_ids (bool, optional): Whether to return conformer IDs. Defaults to ``False``.
 
@@ -63,13 +63,13 @@ def __call__(self,
         Execute the task of pruning conformers.
 
         Args:
-            current_mol_data (list[dict]): conformer data of the current iteration.
-            unique_mol_data (list[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
+            current_mol_data (List[dict]): conformer data of the current iteration.
+            unique_mol_data (List[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
             sort_by_energy (bool, optional): Whether to sort conformers by energy. Defaults to ``True``.
             return_ids (bool, optional): Whether to return conformer IDs. Defaults to ``False``.
 
         Returns:
-            list[dict]: Updated conformer data.
+            List[dict]: Updated conformer data.
         """
         self.iter += 1
         time_start = time()
@@ -172,10 +172,10 @@ def calculate_torsions(self,
         Calculate torsions for a list of conformers.
 
         Args:
-            mol_data (list[dict]): conformer data.
+            mol_data (List[dict]): conformer data.
 
         Returns:
-            list[dict]: Conformer data with values of torsions added.
+            List[dict]: Conformer data with values of torsions added.
         """
         for conf_data in mol_data:
             conf = conf_data["conf"]
@@ -202,7 +202,7 @@ def rad_angle_compare(x: float,
     @staticmethod
     def torsion_list_compare(c1_ts: List[float],
                              c2_ts: List[float],
-                             ) -> list[float]:
+                             ) -> List[float]:
         """
         Compare two lists of torsions in radians.
 
@@ -225,13 +225,13 @@ def prune_conformers(self,
         Prune conformers.
 
         Args:
-            current_mol_data (list[dict]): conformer data of the current iteration.
-            unique_mol_data (list[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
+            current_mol_data (List[dict]): conformer data of the current iteration.
+            unique_mol_data (List[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
             sort_by_energy (bool, optional): Whether to sort conformers by energy. Defaults to ``True``.
             return_ids (bool, optional): Whether to return conformer IDs. Defaults to ``False``.
 
         Returns:
-            list[dict]: Updated conformer data.
+            List[dict]: Updated conformer data.
         """
         if unique_mol_data is None:
             unique_mol_data = []
@@ -322,13 +322,13 @@ def prune_conformers(self,
         Prune conformers.
 
         Args:
-            current_mol_data (list[dict]): conformer data of the current iteration.
-            unique_mol_data (list[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
+            current_mol_data (List[dict]): conformer data of the current iteration.
+            unique_mol_data (List[dict], optional): Unique conformer data of previous iterations. Defaults to ``None``.
             sort_by_energy (bool, optional): Whether to sort conformers by energy. Defaults to ``True``.
             return_ids (bool, optional): Whether to return conformer IDs. Defaults to ``False``.
 
         Returns:
-            list[dict]: Updated conformer data.
+            List[dict]: Updated conformer data.
         """
         if unique_mol_data is None:
             unique_mol_data = []

rdmc/conformer_generation/utils.py

@@ -22,7 +22,7 @@ def mol_to_dict(mol: 'RDKitMol',
                 copy: bool = True,
                 iter: Optional[int] = None,
                 conf_copy_attrs: Optional[list] = None,
-                ) -> list[dict]:
+                ) -> List[dict]:
     """
     Convert a molecule to a dictionary that stores its conformers object, atom coordinates,
     and iteration numbers for a certain calculation (optional).

rdmc/mol.py

@@ -1913,29 +1913,34 @@ def generate_vdw_mat(rd_mol,
 
 def generate_radical_resonance_structures(mol: RDKitMol,
                                           unique: bool = True,
-                                          consider_atommap: bool = True,
+                                          consider_atommap: bool = False,
                                           kekulize: bool = False):
     """
     Generate resonance structures for a radical molecule.  RDKit by design doesn't work
     for radical resonance. The approach is a temporary workaround by replacing radical electrons by positive
     charges and generating resonance structures by RDKit ResonanceMolSupplier.
     Currently, this function only works for neutral radicals.
 
+    Known issues:
+
+    - Phenyl radical only generate one resonance structure when ``kekulize=True``, expecting 2.
+
     Args:
         mol (RDKitMol): A radical molecule.
         unique (bool, optional): Filter out duplicate resonance structures from the list. Defaults to ``True``.
         consider_atommap (bool, atommap): If consider atom map numbers in filtration duplicates.
-                                          Only effective when uniquify=True. Defaults to ``False``.
-        kekulize (bool, optional): Whether to kekulize the molecule. Defaults to ``False``. When ``True``, uniquifying
-                                   process will be skipped.
+                                          Only effective when ``unique=True``. Defaults to ``False``.
+        kekulize (bool, optional): Whether to kekulize the molecule. Defaults to ``False``. As an example,
+                                   benzene have one resonance structure if not kekulized (``False``) and
+                                   two resonance structures if kekulized (``True``).
 
     Returns:
         list: a list of molecules with resonance structures.
     """
     assert mol.GetFormalCharge() == 0, "The current function only works for radical species."
     mol_copy = mol.Copy(quickCopy=True)  # Make a copy of the original molecule
 
-    # Modify the original molecule to make it a postively charged species
+    # Modify the original molecule to make it a positively charged species
     recipe = {}  # Used to record changes. Temporarily not used now.
     for atom in mol_copy.GetAtoms():
         radical_electrons = atom.GetNumRadicalElectrons()
@@ -1957,7 +1962,9 @@ def generate_radical_resonance_structures(mol: RDKitMol,
     mol_copy.UpdatePropertyCache()  # Make sure the assignment is boardcast to atoms / bonds
 
     # Generate Resonance Structures
-    flags = Chem.KEKULE_ALL | Chem.ALLOW_INCOMPLETE_OCTETS | Chem.UNCONSTRAINED_CATIONS
+    flags = Chem.ALLOW_INCOMPLETE_OCTETS | Chem.UNCONSTRAINED_CATIONS
+    if kekulize:
+        flags |= Chem.KEKULE_ALL
     suppl = Chem.ResonanceMolSupplier(mol_copy._mol, flags=flags)
     res_mols = [RDKitMol(RWMol(mol)) for mol in suppl]
 
@@ -1974,38 +1981,48 @@ def generate_radical_resonance_structures(mol: RDKitMol,
             elif charge < 0:  # Shouldn't appear, just for bug detection
                 raise RuntimeError('Encounter charge separation during resonance structure generation.')
 
-            # For aromatic molecules:
-            # Aromaticity flag is incorrectly inherit from the parent molecule
-            # and can cause issues in kekulizing children's structure. Reset all atomic
-            # aromaticity flag does the trick to initiate aromaticity perception in sanitization
-            # Tried other ways but none of them works (e.g., mol.ClearComputedProps())
-            atom.SetIsAromatic(False)
-
         # If a structure cannot be sanitized, removed it
         try:
-            res_mol.Sanitize()
-        except BaseException:
+            # Sanitization strategy is inspired by
+            # https://github.com/rdkit/rdkit/discussions/6358
+            flags = Chem.SanitizeFlags.SANITIZE_ALL
+            if kekulize:
+                flags ^= (Chem.SanitizeFlags.SANITIZE_KEKULIZE | Chem.SanitizeFlags.SANITIZE_SETAROMATICITY)
+            res_mol.Sanitize(sanitizeOps=flags)
+        except BaseException as e:
+            print(e)
             # todo: make error type more specific and add a warning message
             continue
         if kekulize:
-            res_mol.Kekulize()
+            _unset_aromatic_flags(res_mol)
         cleaned_mols.append(res_mol)
 
     # To remove duplicate resonance structures
-    if unique and not kekulize:
+    if unique:
         cleaned_mols = get_unique_mols(cleaned_mols,
                                        consider_atommap=consider_atommap)
-        # Temporary fix to remove highlight flag
-        # TODO: replace with a better method after knowing the mechanism of highlighting substructures
-        cleaned_mols = [RDKitMol.FromSmiles(
-            mol.ToSmiles(removeAtomMap=False,
-                         removeHs=False,
-                         kekule=kekulize,)
-        )
-            for mol in cleaned_mols]
+        for mol in cleaned_mols:
+            # According to
+            # https://github.com/rdkit/rdkit/blob/9249ca5cc840fc72ea3bb73c2ff1d71a1fbd3f47/rdkit/Chem/Draw/IPythonConsole.py#L152
+            # highlight info is stored in __sssAtoms
+            mol._mol.__setattr__('__sssAtoms', [])
     return cleaned_mols
 
 
+def _unset_aromatic_flags(mol):
+    """
+    A helper function to unset aromatic flags in a molecule.
+    This is useful when cleaning up the molecules from resonance structure generation.
+    In such case, a molecule may have single-double bonds but are marked as aromatic bonds.
+    """
+    for bond in mol.GetBonds():
+        if bond.GetBondType() != Chem.BondType.AROMATIC and bond.GetIsAromatic():
+            bond.SetIsAromatic(False)
+            bond.GetBeginAtom().SetIsAromatic(False)
+            bond.GetEndAtom().SetIsAromatic(False)
+    return mol
+
+
 def has_matched_mol(mol: RDKitMol,
                     mols: List[RDKitMol],
                     consider_atommap: bool = False,