From 2899833791538cf8d244de10f7f71168ba67d293 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Wed, 4 Dec 2019 13:06:32 -0500 Subject: [PATCH 1/9] Add more line delimiters to chemkin._remove_line_breaks --- rmgpy/chemkin.pyx | 1 + 1 file changed, 1 insertion(+) diff --git a/rmgpy/chemkin.pyx b/rmgpy/chemkin.pyx index d48c56d061..5d61a30e6e 100644 --- a/rmgpy/chemkin.pyx +++ b/rmgpy/chemkin.pyx @@ -592,6 +592,7 @@ def _remove_line_breaks(comments): 'This direction matched an entry in ', 'From training reaction', 'This reaction matched rate rule', 'family: ', 'Warning:', 'Chemkin file stated explicit reverse rate:', 'Ea raised from', + 'Fitted to', 'Reaction library', ] for indicator in new_statement_indicators: comments = comments.replace(' ' + indicator, '\n' + indicator, 1) From 71abbcbfb561a23ba161381111416bceaf352a4f Mon Sep 17 00:00:00 2001 From: Max Liu Date: Wed, 4 Dec 2019 13:09:08 -0500 Subject: [PATCH 2/9] Remove duplicate get_w0 and get_w0s methods in KineticsFamily Likely left over from move to kinetics.arrhenius Resolves #1826 --- rmgpy/data/kinetics/family.py | 57 ----------------------------------- scripts/rmg2to3.py | 2 -- 2 files changed, 59 deletions(-) diff --git a/rmgpy/data/kinetics/family.py b/rmgpy/data/kinetics/family.py index 416744294f..d6cdcca108 100644 --- a/rmgpy/data/kinetics/family.py +++ b/rmgpy/data/kinetics/family.py @@ -2758,63 +2758,6 @@ def add_atom_labels_for_reaction(self, reaction, output_with_resonance=True): for species in reaction.reactants + reaction.products: species.generate_resonance_structures() - def get_w0(self, rxn): - """ - calculates the w0 for Blower Masel kinetics by calculating wf (total bond energy of bonds formed) - and wb (total bond energy of bonds broken) with w0 = (wf+wb)/2 - """ - mol = None - a_dict = {} - for r in rxn.reactants: - m = r.molecule[0] - a_dict.update(m.get_all_labeled_atoms()) - if mol: - mol = mol.merge(m) - else: - mol = m.copy(deep=True) - - recipe = self.forward_recipe.actions - - wb = 0.0 - wf = 0.0 - for act in recipe: - - if act[0] == 'BREAK_BOND': - bd = mol.get_bond(a_dict[act[1]], a_dict[act[3]]) - wb += bd.get_bde() - elif act[0] == 'FORM_BOND': - bd = Bond(a_dict[act[1]], a_dict[act[3]], act[2]) - wf += bd.get_bde() - elif act[0] == 'CHANGE_BOND': - bd1 = mol.get_bond(a_dict[act[1]], a_dict[act[3]]) - - if act[2] + bd1.order == 0.5: - mol2 = None - for r in rxn.products: - m = r.molecule[0] - if mol2: - mol2 = mol2.merge(m) - else: - mol2 = m.copy(deep=True) - bd2 = mol2.get_bond(a_dict[act[1]], a_dict[act[3]]) - else: - bd2 = Bond(a_dict[act[1]], a_dict[act[3]], bd1.order + act[2]) - - if bd2.order == 0: - bd2_bde = 0.0 - else: - bd2_bde = bd2.get_bde() - bde_diff = bd2_bde - bd1.get_bde() - if bde_diff > 0: - wf += abs(bde_diff) - else: - wb += abs(bde_diff) - - return (wf + wb) / 2.0 - - def get_w0s(self, rxns): - return list(map(self.get_w0, rxns)) - def get_training_depository(self): """ Returns the `training` depository from self.depositories diff --git a/scripts/rmg2to3.py b/scripts/rmg2to3.py index 59272f68c3..0f06e7f364 100644 --- a/scripts/rmg2to3.py +++ b/scripts/rmg2to3.py @@ -826,8 +826,6 @@ 'retrieveTemplate': 'retrieve_template', 'getLabeledReactantsAndProducts': 'get_labeled_reactants_and_products', 'addAtomLabelsForReaction': 'add_atom_labels_for_reaction', - 'getw0': 'get_w0', - 'getw0s': 'get_w0s', 'getTrainingDepository': 'get_training_depository', 'addEntry': 'add_entry', # rmgpy.data.kinetics.library From e926c06763dc0be195b5e1919669244bb650fcc5 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Wed, 4 Dec 2019 13:11:43 -0500 Subject: [PATCH 3/9] Do not calculate error from thermo model conversion We were not doing anything with the results, so it was doing pointless computations. Resolves #1812 --- rmgpy/thermo/thermoengine.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/rmgpy/thermo/thermoengine.py b/rmgpy/thermo/thermoengine.py index 92e10fc3e8..3ad68030de 100644 --- a/rmgpy/thermo/thermoengine.py +++ b/rmgpy/thermo/thermoengine.py @@ -98,15 +98,6 @@ def process_thermo_data(spc, thermo0, thermo_class=NASA, solvent_name=''): else: raise Exception('thermo_class neither NASA nor Wilhoit. Cannot process thermo data.') - if thermo.__class__ != thermo0.__class__: - # Compute RMS error of overall transformation - Tlist = np.array([300.0, 400.0, 500.0, 600.0, 800.0, 1000.0, 1500.0], np.float64) - err = 0.0 - for T in Tlist: - err += (thermo.get_heat_capacity(T) - thermo0.get_heat_capacity(T)) ** 2 - err = math.sqrt(err / len(Tlist)) / constants.R - # logging.log(logging.WARNING if err > 0.2 else 0, 'Average RMS error in heat capacity fit to {0} = {1:g}*R'.format(spc, err)) - return thermo From 7d2e60c69089faadcc70e4f8be47f007de7fffc6 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Wed, 4 Dec 2019 13:59:10 -0500 Subject: [PATCH 4/9] Add logging of molecule adjlist when identifier generation fails This makes it easier to figure out the problematic molecule --- rmgpy/molecule/translator.py | 2 ++ rmgpy/molecule/translatorTest.py | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/rmgpy/molecule/translator.py b/rmgpy/molecule/translator.py index 66eed92ebe..9a9f35b92e 100644 --- a/rmgpy/molecule/translator.py +++ b/rmgpy/molecule/translator.py @@ -531,6 +531,8 @@ def _write(mol, identifier_type, backend): logging.debug('Backend {0} is not able to generate {1} for this molecule:\n' '{2}'.format(option, identifier_type, mol.to_adjacency_list())) + logging.error('Unable to generate identifier for this molecule:\n{0}'.format(mol.to_adjacency_list())) + raise ValueError("Unable to generate identifier type {0} with backend {1}.".format(identifier_type, backend)) diff --git a/rmgpy/molecule/translatorTest.py b/rmgpy/molecule/translatorTest.py index d60f588d6a..9506b45ae5 100644 --- a/rmgpy/molecule/translatorTest.py +++ b/rmgpy/molecule/translatorTest.py @@ -34,6 +34,7 @@ import re import unittest from external.wip import work_in_progress +from unittest.mock import patch from rmgpy.molecule.adjlist import ConsistencyChecker from rmgpy.molecule.atomtype import ATOMTYPES @@ -52,6 +53,18 @@ def test_empty_molecule(self): self.assertEqual(mol.to_smiles(), '') self.assertEqual(mol.to_inchi(), '') + @patch('rmgpy.molecule.translator.logging') + def test_failure_message(self, mock_logging): + """Test that we log the molecule adjlist upon failure.""" + mol = Molecule(smiles='[CH2-][N+]#N') + + with self.assertRaisesRegex(ValueError, 'Unable to generate identifier type'): + to_inchi(mol, backend='rdkit') + + mock_logging.error.assert_called_with( + "Unable to generate identifier for this molecule:\n{0}".format(mol.to_adjacency_list()) + ) + class InChIGenerationTest(unittest.TestCase): def compare(self, adjlist, aug_inchi): From 987cf7449750a2a47cef8fd9d0528a9b0fcab797 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Wed, 4 Dec 2019 14:30:29 -0500 Subject: [PATCH 5/9] Add support for converting surface species to obmol This enables identifier generation for surface species using OpenBabel as the backend Copies the approach used for RDKit by replacing X with Pt --- rmgpy/molecule/converter.py | 5 ++- rmgpy/molecule/translatorTest.py | 52 ++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/rmgpy/molecule/converter.py b/rmgpy/molecule/converter.py index 808e077307..675ecdc5fa 100644 --- a/rmgpy/molecule/converter.py +++ b/rmgpy/molecule/converter.py @@ -226,7 +226,10 @@ def to_ob_mol(mol, return_mapping=False): obmol = openbabel.OBMol() for atom in atoms: a = obmol.NewAtom() - a.SetAtomicNum(atom.number) + if atom.element.symbol == 'X': + a.SetAtomicNum(78) # not sure how to do this with linear scaling when this might not be Pt + else: + a.SetAtomicNum(atom.number) if atom.element.isotope != -1: a.SetIsotope(atom.element.isotope) a.SetFormalCharge(atom.charge) diff --git a/rmgpy/molecule/translatorTest.py b/rmgpy/molecule/translatorTest.py index 9506b45ae5..1625dbfdde 100644 --- a/rmgpy/molecule/translatorTest.py +++ b/rmgpy/molecule/translatorTest.py @@ -427,6 +427,32 @@ def test_isotopic_molecule_2(self): self.assertEqual(mol.to_inchi(), inchi) + def test_surface_molecule_rdkit(self): + """Test InChI generation for a surface molecule using RDKit""" + mol = Molecule().from_adjacency_list(""" +1 C u0 p0 c0 {2,S} {3,S} {4,S} {5,S} +2 H u0 p0 c0 {1,S} +3 H u0 p0 c0 {1,S} +4 H u0 p0 c0 {1,S} +5 X u0 p0 c0 {1,S} +""") + inchi = 'InChI=1S/CH3.Pt/h1H3;' + + self.assertEqual(to_inchi(mol, backend='rdkit'), inchi) + + def test_surface_molecule_ob(self): + """Test InChI generation for a surface molecule using OpenBabel""" + mol = Molecule().from_adjacency_list(""" +1 C u0 p0 c0 {2,S} {3,S} {4,S} {5,S} +2 H u0 p0 c0 {1,S} +3 H u0 p0 c0 {1,S} +4 H u0 p0 c0 {1,S} +5 X u0 p0 c0 {1,S} +""") + inchi = 'InChI=1S/CH3.Pt/h1H3;' + + self.assertEqual(to_inchi(mol, backend='openbabel'), inchi) + class SMILESGenerationTest(unittest.TestCase): def compare(self, adjlist, smiles): @@ -754,6 +780,32 @@ def test_aromatics(self): self.assertNotEqual(smiles2, smiles3) self.assertNotEqual(smiles1, smiles3) + def test_surface_molecule_rdkit(self): + """Test InChI generation for a surface molecule using RDKit""" + mol = Molecule().from_adjacency_list(""" +1 C u0 p0 c0 {2,S} {3,S} {4,S} {5,S} +2 H u0 p0 c0 {1,S} +3 H u0 p0 c0 {1,S} +4 H u0 p0 c0 {1,S} +5 X u0 p0 c0 {1,S} +""") + smiles = 'C[Pt]' + + self.assertEqual(to_smiles(mol, backend='rdkit'), smiles) + + def test_surface_molecule_ob(self): + """Test InChI generation for a surface molecule using OpenBabel""" + mol = Molecule().from_adjacency_list(""" +1 C u0 p0 c0 {2,S} {3,S} {4,S} {5,S} +2 H u0 p0 c0 {1,S} +3 H u0 p0 c0 {1,S} +4 H u0 p0 c0 {1,S} +5 X u0 p0 c0 {1,S} +""") + smiles = 'C[Pt]' + + self.assertEqual(to_smiles(mol, backend='openbabel'), smiles) + class ParsingTest(unittest.TestCase): def setUp(self): From 1655008eeaf403c4015fefac7ccc5e384faf36bf Mon Sep 17 00:00:00 2001 From: Max Liu Date: Wed, 4 Dec 2019 15:56:47 -0500 Subject: [PATCH 6/9] Change classmethod args from self to cls --- rmgpy/data/thermoTest.py | 14 +++++++------- rmgpy/data/transportTest.py | 10 +++++----- rmgpy/rmg/rmgTest.py | 20 ++++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/rmgpy/data/thermoTest.py b/rmgpy/data/thermoTest.py index 3248443d26..1bd88a5ba7 100644 --- a/rmgpy/data/thermoTest.py +++ b/rmgpy/data/thermoTest.py @@ -82,19 +82,19 @@ class TestThermoDatabase(unittest.TestCase): """ @classmethod - def setUpClass(self): + def setUpClass(cls): """A function that is run ONCE before all unit tests in this class.""" global database - self.database = database.thermo + cls.database = database.thermo - self.databaseWithoutLibraries = ThermoDatabase() - self.databaseWithoutLibraries.load(os.path.join(settings['database.directory'], 'thermo'), libraries=[]) + cls.databaseWithoutLibraries = ThermoDatabase() + cls.databaseWithoutLibraries.load(os.path.join(settings['database.directory'], 'thermo'), libraries=[]) # Set up ML estimator models_path = os.path.join(settings['database.directory'], 'thermo', 'ml', 'main') hf298_path = os.path.join(models_path, 'hf298') s298_cp_path = os.path.join(models_path, 's298_cp') - self.ml_estimator = MLEstimator(hf298_path, s298_cp_path) + cls.ml_estimator = MLEstimator(hf298_path, s298_cp_path) def test_pickle(self): """ @@ -1640,10 +1640,10 @@ class TestThermoCentralDatabaseInterface(unittest.TestCase): """ @classmethod - def setUpClass(self): + def setUpClass(cls): """A function that is run ONCE before all unit tests in this class.""" global database - self.database = database.thermo + cls.database = database.thermo def connect_to_test_central_database(self): host, port, username, password = get_testing_tcd_authentication_info() diff --git a/rmgpy/data/transportTest.py b/rmgpy/data/transportTest.py index 4bbf5da445..3547130499 100644 --- a/rmgpy/data/transportTest.py +++ b/rmgpy/data/transportTest.py @@ -126,13 +126,13 @@ class TestTransportDatabase(unittest.TestCase): """ @classmethod - def setUpClass(self): + def setUpClass(cls): """A function that is run ONCE before all unit tests in this class.""" - self.database = TransportDatabase() - self.database.load(os.path.join(settings['database.directory'], 'transport'), - ['GRI-Mech', 'PrimaryTransportLibrary']) + cls.database = TransportDatabase() + cls.database.load(os.path.join(settings['database.directory'], 'transport'), + ['GRI-Mech', 'PrimaryTransportLibrary']) - self.speciesList = [ + cls.speciesList = [ Species().from_smiles('C'), Species().from_smiles('CCCC'), Species().from_smiles('O'), diff --git a/rmgpy/rmg/rmgTest.py b/rmgpy/rmg/rmgTest.py index c7eac0f953..f7f09ecf30 100644 --- a/rmgpy/rmg/rmgTest.py +++ b/rmgpy/rmg/rmgTest.py @@ -47,30 +47,30 @@ class TestRMGWorkFlow(unittest.TestCase): @classmethod - def setUpClass(self): + def setUpClass(cls): """ A method that is run before all unit tests in this class. """ # set-up RMG object - self.rmg = RMG() - self.rmg.reaction_model = CoreEdgeReactionModel() + cls.rmg = RMG() + cls.rmg.reaction_model = CoreEdgeReactionModel() # load kinetic database and forbidden structures - self.rmg.database = RMGDatabase() + cls.rmg.database = RMGDatabase() path = os.path.join(settings['test_data.directory'], 'testing_database') # kinetics family Disproportionation loading - self.rmg.database.load_kinetics(os.path.join(path, 'kinetics'), - kinetics_families=['H_Abstraction', 'R_Addition_MultipleBond'], - reaction_libraries=[]) + cls.rmg.database.load_kinetics(os.path.join(path, 'kinetics'), + kinetics_families=['H_Abstraction', 'R_Addition_MultipleBond'], + reaction_libraries=[]) # load empty forbidden structures - for family in self.rmg.database.kinetics.families.values(): + for family in cls.rmg.database.kinetics.families.values(): family.forbidden = ForbiddenStructures() - self.rmg.database.forbidden_structures = ForbiddenStructures() + cls.rmg.database.forbidden_structures = ForbiddenStructures() @classmethod - def tearDownClass(self): + def tearDownClass(cls): """ Reset the loaded database """ From 08e8a1d237c317ae275ec1351ec028c79e878c4e Mon Sep 17 00:00:00 2001 From: Max Liu Date: Fri, 6 Dec 2019 14:37:09 -0500 Subject: [PATCH 7/9] Increase tolerance for rate check in pdep test Resolves #1682 --- arkane/pdepTest.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arkane/pdepTest.py b/arkane/pdepTest.py index 8081372b33..ff1f910d3d 100644 --- a/arkane/pdepTest.py +++ b/arkane/pdepTest.py @@ -104,7 +104,9 @@ def test_pdep_job(self): reaction_list = read_reactions_block(chem, dictionary) rxn = reaction_list[0] self.assertIsInstance(rxn.kinetics, Chebyshev) - self.assertAlmostEquals(rxn.kinetics.get_rate_coefficient(1000.0, 1.0), 88.88253229631246) + # Accept a delta of 0.2, which could result from numerical discrepancies + # See RMG-Py #1682 on GitHub for discussion + self.assertAlmostEquals(rxn.kinetics.get_rate_coefficient(1000.0, 1.0), 88.88253229631246, delta=0.2) files = [f for f in os.listdir(os.path.join(self.directory, 'sensitivity', '')) if os.path.isfile(os.path.join(self.directory, 'sensitivity', f))] @@ -116,7 +118,7 @@ def test_pdep_job(self): if '1000.0' in line: break sa_coeff = line.split()[-2] - self.assertEquals(float(sa_coeff), -8.23e-6) + self.assertAlmostEquals(float(sa_coeff), -8.23e-6, delta=0.02e-6) @classmethod def tearDown(cls): From 58fce35ccb75c12fc4941673a78e0faa92fa57e7 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Fri, 6 Dec 2019 14:50:09 -0500 Subject: [PATCH 8/9] Replace mock with unittest.mock mock was added into the standard library in Python 3.3 --- .conda/meta.yaml | 1 - environment.yml | 1 - rmgpy/chemkinTest.py | 4 ++-- rmgpy/constraintsTest.py | 2 +- rmgpy/data/kinetics/familyTest.py | 2 +- 5 files changed, 4 insertions(+), 6 deletions(-) diff --git a/.conda/meta.yaml b/.conda/meta.yaml index 0bcf89d13d..4177b709b9 100644 --- a/.conda/meta.yaml +++ b/.conda/meta.yaml @@ -43,7 +43,6 @@ requirements: - lpsolve55 - markupsafe - matplotlib >=1.5 - - mock - mopac - mpmath - networkx diff --git a/environment.yml b/environment.yml index b514e1ef59..1c4faca9df 100644 --- a/environment.yml +++ b/environment.yml @@ -24,7 +24,6 @@ dependencies: - lpsolve55 - markupsafe - matplotlib >=1.5 - - mock - mopac - mpmath - networkx diff --git a/rmgpy/chemkinTest.py b/rmgpy/chemkinTest.py index 59614fd8f2..f2b295ba3b 100644 --- a/rmgpy/chemkinTest.py +++ b/rmgpy/chemkinTest.py @@ -27,9 +27,9 @@ # # ############################################################################### -import unittest -import mock import os +import unittest +from unittest import mock import rmgpy from rmgpy.chemkin import get_species_identifier, load_chemkin_file, load_transport_file, mark_duplicate_reactions, \ diff --git a/rmgpy/constraintsTest.py b/rmgpy/constraintsTest.py index d917178807..5f002d9ea8 100644 --- a/rmgpy/constraintsTest.py +++ b/rmgpy/constraintsTest.py @@ -32,7 +32,7 @@ """ import unittest -import mock +from unittest import mock from rmgpy.rmg.main import RMG from rmgpy.constraints import fails_species_constraints diff --git a/rmgpy/data/kinetics/familyTest.py b/rmgpy/data/kinetics/familyTest.py index 9f39fdeaa2..abf087c184 100644 --- a/rmgpy/data/kinetics/familyTest.py +++ b/rmgpy/data/kinetics/familyTest.py @@ -32,8 +32,8 @@ import os.path import shutil import unittest +from unittest import mock -import mock import numpy as np from rmgpy import settings From 8748f7335dcc8a6f114f97e00b8826e858d191b8 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Fri, 6 Dec 2019 15:05:15 -0500 Subject: [PATCH 9/9] Minor PEP-8 fixes to arrhenius.get_w0 --- rmgpy/kinetics/arrhenius.pyx | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/rmgpy/kinetics/arrhenius.pyx b/rmgpy/kinetics/arrhenius.pyx index ae519ddbd9..9925fd1b28 100644 --- a/rmgpy/kinetics/arrhenius.pyx +++ b/rmgpy/kinetics/arrhenius.pyx @@ -1081,10 +1081,10 @@ def get_w0(actions, rxn): and wb (total bond energy of bonds broken) with w0 = (wf+wb)/2 """ mol = None - aDict = {} + a_dict = {} for r in rxn.reactants: m = r.molecule[0] - aDict.update(m.get_all_labeled_atoms()) + a_dict.update(m.get_all_labeled_atoms()) if mol: mol = mol.merge(m) else: @@ -1097,13 +1097,13 @@ def get_w0(actions, rxn): for act in recipe: if act[0] == 'BREAK_BOND': - bd = mol.get_bond(aDict[act[1]], aDict[act[3]]) + bd = mol.get_bond(a_dict[act[1]], a_dict[act[3]]) wb += bd.get_bde() elif act[0] == 'FORM_BOND': - bd = Bond(aDict[act[1]], aDict[act[3]], act[2]) + bd = Bond(a_dict[act[1]], a_dict[act[3]], act[2]) wf += bd.get_bde() elif act[0] == 'CHANGE_BOND': - bd1 = mol.get_bond(aDict[act[1]], aDict[act[3]]) + bd1 = mol.get_bond(a_dict[act[1]], a_dict[act[3]]) if act[2] + bd1.order == 0.5: mol2 = None @@ -1113,19 +1113,19 @@ def get_w0(actions, rxn): mol2 = mol2.merge(m) else: mol2 = m.copy(deep=True) - bd2 = mol2.get_bond(aDict[act[1]], aDict[act[3]]) + bd2 = mol2.get_bond(a_dict[act[1]], a_dict[act[3]]) else: - bd2 = Bond(aDict[act[1]], aDict[act[3]], bd1.order + act[2]) + bd2 = Bond(a_dict[act[1]], a_dict[act[3]], bd1.order + act[2]) if bd2.order == 0: - bd2bde = 0.0 + bd2_bde = 0.0 else: - bd2bde = bd2.get_bde() - bdediff = bd2bde - bd1.get_bde() - if bdediff > 0: - wf += abs(bdediff) + bd2_bde = bd2.get_bde() + bde_diff = bd2_bde - bd1.get_bde() + if bde_diff > 0: + wf += abs(bde_diff) else: - wb += abs(bdediff) + wb += abs(bde_diff) return (wf + wb) / 2.0 def get_w0s(actions, rxns):