xiaoruiDong / RDMC

Reaction Data and Molecular Conformers (RDMC) is a package dealing with reactions, molecules, conformers, majorly in 3D.
https://xiaoruidong.github.io/RDMC/
MIT License
23 stars 1 forks source link

Save energy of each optimized TS in .sdf file #38

Closed shihchengli closed 2 years ago

shihchengli commented 2 years ago

This addition allows the energy of each optimized TS to be saved in the .sdf file.

xiaoruiDong commented 2 years ago

Looks good to me. Shih-Cheng, can you try to refactor save_opt_mol a bit?

 def save_opt_mols(self,
                      save_dir: str,
                      opt_mol: 'RDKitMol',
                      keep_ids: dict = None,
                      energies: dict = None,
                      ):
        """
        Save the information of the optimized TS geometries into the directory.

        Args:
            save_dir (str): The path to the directory to save the results.
            opt_mol (RDKitMol): The optimized TS molecule in RDKitMol with 3D conformer saved with the molecule.
            keep_ids (dict): Dictionary of which opts succeeded and which failed
        """
        # Save optimized ts mols
        ts_path = os.path.join(save_dir, "ts_optimized_confs.sdf")
        try:
            ts_writer = Chem.rdmolfiles.SDWriter(ts_path)
            for i in range(opt_mol.GetNumConformers()):
                ts_writer.write(opt_mol, confId=i)
        except Exception:
            raise
        finally:
            ts_writer.close()

        # save ids
        with open(os.path.join(save_dir, "opt_check_ids.pkl"), "wb") as f:
            pickle.dump(keep_ids, f)
xiaoruiDong commented 2 years ago

Thanks @shihchengli, looks good to me. @PattanaikL The function save_opt_mols needs to be refactorized later, though.

  1. It should be decorated by @staticmethod as self doesn't play a role in the function
  2. I don't know why opt_mol.ToRWMol() needs to be done when calling the function save_opt_mol instead of calling inside the function save_opt_mol. In that case, keep_ids, and energies can be two optional arguments. If not provided, then try to get them from opt_mol.energies and opt_mol.KeepIDs

Anyway, I will merge this PR and solve that later.