Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collection of small fixes #1842

Merged
merged 9 commits into from
Dec 6, 2019
1 change: 0 additions & 1 deletion .conda/meta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ requirements:
- lpsolve55
- markupsafe
- matplotlib >=1.5
- mock
- mopac
- mpmath
- networkx
Expand Down
6 changes: 4 additions & 2 deletions arkane/pdepTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about making this a percentage test.

self.assertLess((abs(rxn.kinetics.get_rate_coefficient(1000.0, 1.0) - 88.88253229631246)) / 88.88253229631246,
                        0.01)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was disappointed that assertAlmostEquals did not support a percentage delta, but I think I would prefer the simplicity of using it with a fixed delta over calculating the percentage. I could change the delta to 0.01*88.88 if you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that is okay, we can leave it as is then. I do like the simplicity of just using delta


files = [f for f in os.listdir(os.path.join(self.directory, 'sensitivity', ''))
if os.path.isfile(os.path.join(self.directory, 'sensitivity', f))]
Expand All @@ -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):
Expand Down
1 change: 0 additions & 1 deletion environment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ dependencies:
- lpsolve55
- markupsafe
- matplotlib >=1.5
- mock
- mopac
- mpmath
- networkx
Expand Down
1 change: 1 addition & 0 deletions rmgpy/chemkin.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions rmgpy/chemkinTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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, \
Expand Down
2 changes: 1 addition & 1 deletion rmgpy/constraintsTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
57 changes: 0 additions & 57 deletions rmgpy/data/kinetics/family.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions are also listed under rmgpy.data.kinetics.family in rmg2to3.py. I am not sure if this needs to be updated, as what you really want is for these functions to instead call Arrhenius. But I doubt this will come up anyways

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the function has existed long enough for anyone to have scripts using it (except Matt). I added a change to remove them from rmg2to3.py.

"""
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 = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and these functions are essentially the same, but the one we are keeping in arrhenius.pyx has a few additional PEP-8 changes. Can you make these changes?

  • ADict --> a_dict
  • Opitionally, things like bd2bde --> bd2_bde. There are a couple of these variables that I different, but it is PEP-8 as is, so only change if you prefer it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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
Expand Down
2 changes: 1 addition & 1 deletion rmgpy/data/kinetics/familyTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 7 additions & 7 deletions rmgpy/data/thermoTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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()
Expand Down
10 changes: 5 additions & 5 deletions rmgpy/data/transportTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
26 changes: 13 additions & 13 deletions rmgpy/kinetics/arrhenius.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand All @@ -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):
Expand Down
5 changes: 4 additions & 1 deletion rmgpy/molecule/converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions rmgpy/molecule/translator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead add the adjlist to the error message below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing that originally. One consideration is that with the current setup, logging goes into RMG.log, while the error goes into stderr, which is only printed to the terminal (or slurm/sge output files). I can move it though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, then let's keep it. I'm OK with all other commits too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not be relevant here but the discussion reminded me: I am a fan of using logging.exception() in an exception handler to add context and make sure the original stack trace ends up in the log. Can then raise() the original exception as if you hadn’t caught it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not be relevant here but the discussion reminded me: I am a fan of using logging.exception() in an exception handler to add context and make sure the original stack trace ends up in the log. Can then raise() the original exception as if you hadn’t caught it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was actually hoping that this would be a good opportunity to use logging.exception(), but that has to be called from an exception handler. It seemed easier to log this message once in this function rather than logging the exception in each function which calls it.

I do think there are a lot of instances where we use logging.error when catching exceptions where logging.exception could be used instead.


raise ValueError("Unable to generate identifier type {0} with backend {1}.".format(identifier_type, backend))


Expand Down
65 changes: 65 additions & 0 deletions rmgpy/molecule/translatorTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -414,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):
Expand Down Expand Up @@ -741,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):
Expand Down
Loading