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

A collection of Matt and David's commits from #2316 #2491

Merged
merged 24 commits into from
Jul 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
34ed5a3
add functionality for constant species in Reactor classes
mjohnson541 Sep 4, 2021
a6742e9
For surface species calculate solvent correction from the saturated d…
mjohnson541 Sep 4, 2021
49bb3c1
added `max_surface_bond_order` constraint
davidfarinajr Sep 2, 2021
7cee142
fix bug with max_surface_sites specification
mjohnson541 Oct 4, 2021
5badbd1
handle case when symbol is in all caps
mjohnson541 Oct 4, 2021
92fad47
make it possible to explicitly forbid molecules and groups in the inp…
mjohnson541 Oct 4, 2021
54d9e72
fig adjacencyListGroup in input
mjohnson541 May 7, 2023
bd1d25d
Revert "make it possible to explicitly forbid molecules and groups in…
rwest Jul 14, 2023
20adbd1
add comment attribute to species and reactions in rms yaml file
mjohnson541 Jan 11, 2022
4425ac5
when rule making is done on one processor don't use pool
mjohnson541 Jan 11, 2022
7d527a3
add function for averaging kinetics
mjohnson541 Jan 11, 2022
23d88f2
check reaction exists before checking coverage_dependence
mjohnson541 Jun 18, 2022
0a6c456
Replace Asserts with ValueErrors
rwest Jul 2, 2023
2180175
Remove charge transfer types from average_kinetics (to be reverted)
rwest Jul 2, 2023
9609275
Unit test for average_kinetics method
rwest Jul 4, 2023
88adf99
Add documentation (examples) for recently added species constraints.
rwest Jul 14, 2023
f8fdf43
Check species constraints in consistent order.
rwest Jul 14, 2023
f9991fe
Unit test for maximumSurfaceBondOrder species constraint.
rwest Jul 14, 2023
25ff548
Changing 'or or' to 'in set' checks.
rwest Jul 14, 2023
409b39d
Remove obsolete comments.
rwest Jul 14, 2023
6071531
Update attribute name in a comment.
rwest Jul 14, 2023
09a39e9
Make check explicitly 'not None'.
rwest Jul 14, 2023
4013707
Comment an elegant alternative averaging scheme.
rwest Jul 14, 2023
cfe970f
Simplify the atomic symbol to number conversion.
rwest Jul 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion arkane/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,11 @@ def get_element_mass(input_element, isotope=None):
number = input_element
elif isinstance(input_element, str):
symbol = input_element
number = next(key for key, value in symbol_by_number.items() if value == input_element)
try:
rwest marked this conversation as resolved.
Show resolved Hide resolved
number = number_by_symbol[symbol]
except KeyError:
symbol = input_element.capitalize()
number = number_by_symbol[symbol]

if symbol is None or number is None:
raise ValueError('Could not identify element {0}'.format(input_element))
Expand Down Expand Up @@ -434,6 +438,7 @@ def get_element_mass(input_element, isotope=None):
92: 'U', 93: 'Np', 94: 'Pu', 95: 'Am', 96: 'Cm', 97: 'Bk', 98: 'Cf', 99: 'Es', 100: 'Fm', 101: 'Md',
102: 'No', 103: 'Lr', 104: 'Rf', 105: 'Db', 106: 'Sg', 107: 'Bh', 108: 'Hs', 109: 'Mt', 110: 'Ds',
111: 'Rg', 112: 'Cn', 113: 'Nh', 114: 'Fl', 115: 'Mc', 116: 'Lv', 117: 'Ts', 118: 'Og'}
number_by_symbol = {value: key for key, value in symbol_by_number.items()}

# Structure of mass_by_symbol items: list(list(isotope1, mass1, weight1), list(isotope2, mass2, weight2), ...)
mass_by_symbol = {
Expand Down
1 change: 1 addition & 0 deletions arkane/commonTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ def test_get_mass(self):
"""Test that the correct mass/number/isotope is returned from get_element_mass"""
self.assertEqual(get_element_mass(1), (1.00782503224, 1)) # test input by integer
self.assertEqual(get_element_mass('Si'), (27.97692653465, 14)) # test string input and most common isotope
self.assertEqual(get_element_mass('SI'), (27.97692653465, 14)) # test string in all caps
self.assertEqual(get_element_mass('C', 13), (13.00335483507, 6)) # test specific isotope
self.assertEqual(get_element_mass('Bk'), (247.0703073, 97)) # test a two-element array (no isotope data)

Expand Down
1 change: 1 addition & 0 deletions documentation/source/users/rmg/input.rst
Original file line number Diff line number Diff line change
Expand Up @@ -989,6 +989,7 @@ all of RMG's reaction families. ::
maximumSulfurAtoms=2,
maximumHeavyAtoms=10,
maximumSurfaceSites=2,
maximumSurfaceBondOrder=4,
maximumRadicalElectrons=2,
maximumSingletCarbenes=1,
maximumCarbeneRadicals=0,
Expand Down
2 changes: 2 additions & 0 deletions examples/rmg/commented/input.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,8 @@
maximumNitrogenAtoms=0,
maximumSiliconAtoms=0,
maximumSulfurAtoms=0,
maximumSurfaceSites=2, # maximum number of surface sites (for heterogeneous catalysis)
maximumSurfaceBondOrder=2, # maximum bond order of each surface sites (for heterogeneous catalysis)
# max number of non-hydrogen atoms
# maximumHeavyAtoms=20,
# maximum radicals on a molecule
Expand Down
14 changes: 10 additions & 4 deletions rmgpy/constraints.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,21 @@ def fails_species_constraints(species):
if struct.get_num_atoms('S') > max_sulfur_atoms:
return True

max_heavy_atoms = species_constraints.get('maximumHeavyAtoms', -1)
if max_heavy_atoms != -1:
if struct.get_num_atoms() - struct.get_num_atoms('H') > max_heavy_atoms:
return True

max_surface_sites = species_constraints.get('maximumSurfaceSites', -1)
if max_surface_sites != -1:
if struct.get_num_atoms('X') > max_surface_sites:
return True

max_heavy_atoms = species_constraints.get('maximumHeavyAtoms', -1)
if max_heavy_atoms != -1:
if struct.get_num_atoms() - struct.get_num_atoms('H') > max_heavy_atoms:
return True
max_surface_bond_order = species_constraints.get('maximumSurfaceBondOrder', -1)
if max_surface_bond_order != -1:
for site in struct.get_surface_sites():
if site.get_total_bond_order() > max_surface_bond_order:
return True

max_radicals = species_constraints.get('maximumRadicalElectrons', -1)
if max_radicals != -1:
Expand Down
11 changes: 11 additions & 0 deletions rmgpy/constraintsTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def setUpClass(cls):
maximumSiliconAtoms=1,
maximumSulfurAtoms=1,
maximumSurfaceSites=2,
maximumSurfaceBondOrder=3,
maximumHeavyAtoms=3,
maximumRadicalElectrons=2,
maximumSingletCarbenes=1,
Expand Down Expand Up @@ -211,6 +212,16 @@ def test_surface_site_constraint(self):
self.rmg.species_constraints['maximumCarbonAtoms'] = max_carbon
self.rmg.species_constraints['maximumHeavyAtoms'] = max_heavy_atoms

def test_surface_bond_order_constraint(self):
"""
Test that we can constrain the max bond order of surface sites.
"""
mol_1site = Molecule().from_adjacency_list("""
1 C u0 p0 c0 {2,Q}
2 X u0 p0 c0 {1,Q}
""")
self.assertTrue(fails_species_constraints(mol_1site))

def test_heavy_constraint(self):
"""
Test that we can constrain the max number of heavy atoms.
Expand Down
65 changes: 63 additions & 2 deletions rmgpy/data/kinetics/family.py
Original file line number Diff line number Diff line change
Expand Up @@ -3621,9 +3621,12 @@
inds = inds.tolist()
revinds = [inds.index(x) for x in np.arange(len(inputs))]

pool = mp.Pool(nprocs)
if nprocs > 1:
pool = mp.Pool(nprocs)
kinetics_list = np.array(pool.map(_make_rule, inputs[inds]))

Check warning on line 3626 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L3625-L3626

Added lines #L3625 - L3626 were not covered by tests
else:
kinetics_list = np.array(list(map(_make_rule, inputs[inds])))

kinetics_list = np.array(pool.map(_make_rule, inputs[inds]))
kinetics_list = kinetics_list[revinds] # fix order

for i, kinetics in enumerate(kinetics_list):
Expand Down Expand Up @@ -4670,3 +4673,61 @@
extension_iter_max=extension_iter_max, extension_iter_item_cap=extension_iter_item_cap)

child_conn.send(list(family.groups.entries.values()))

def average_kinetics(kinetics_list):
"""
Based on averaging log k.
Hence we average n, Ea, arithmetically, but we
average log A (geometric average)
"""
logA = 0.0
n = 0.0
Ea = 0.0
count = 0
for kinetics in kinetics_list:
count += 1
logA += np.log10(kinetics.A.value_si)
n += kinetics.n.value_si
Ea += kinetics.Ea.value_si

logA /= count
n /= count
Ea /= count
Comment on lines +4686 to +4695
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just use Python's built in average, or numpy's. i.e. np.mean([i.n.value_si for i in kinetics_list]) (and include the log10 for geometric, or use a method that just does geometric mean).

Copy link
Member Author

Choose a reason for hiding this comment

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

But we'd have to iterate through the list multiple times (once for each attribute) and create a bunch of lists. Is this a verbosity thing? an efficiency thing? a less-likely-to-make-mistakes thing?
I don't like playing code golf for the sake of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we'd have to iterate through the list multiple times (once for each attribute) and create a bunch of lists.

You can do it very efficiently in one line like this:

from types import SimpleNamespace
import numpy as np

# mimic the kinetics objects
kinetics_list = [SimpleNamespace(a=1, b=2, c=3) for _ in range(3)]

a, b, c = np.mean([[np.log10(i.a), i.b, i.c] for i in kinetics_list], axis=1)

Is this a verbosity thing? an efficiency thing? a less-likely-to-make-mistakes thing? I don't like playing code golf for the sake of it.

This is an antipattern, which we benefit from removing for all of these reasons. Numpy will be faster, less code is easier to maintain and read, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

A hole in one!

I quite like it. Although, having now taught programming a few times, I am learning to appreciate the benefits of code that a novice can understand at a glance. More participation, fewer mistakes, less time spent explaining or parsing, etc. etc.

Anyway, the main reason to not do it your way here and now is that we soon need to revert this commit
2180175
and the merge conflicts would be horrible to resolve.

We can revisit optimizing this code after that happens.
(Though I think this code is seldom called)


## The above could be replaced with something like:
# logA, n, Ea = np.mean([[np.log10(k.A.value_si),
# k.n.value_si,
# k.Ea.value_si] for k in kinetics_list], axis=1)

Aunits = kinetics_list[0].A.units
if Aunits in {'cm^3/(mol*s)', 'cm^3/(molecule*s)', 'm^3/(molecule*s)'}:
Aunits = 'm^3/(mol*s)'
elif Aunits in {'cm^6/(mol^2*s)', 'cm^6/(molecule^2*s)', 'm^6/(molecule^2*s)'}:
Aunits = 'm^6/(mol^2*s)'

Check warning on line 4706 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L4706

Added line #L4706 was not covered by tests
elif Aunits in {'s^-1', 'm^3/(mol*s)', 'm^6/(mol^2*s)'}:
# they were already in SI
pass

Check warning on line 4709 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L4709

Added line #L4709 was not covered by tests
elif Aunits in {'m^2/(mol*s)', 'cm^2/(mol*s)', 'm^2/(molecule*s)', 'cm^2/(molecule*s)'}:
# surface: bimolecular (Langmuir-Hinshelwood)
Aunits = 'm^2/(mol*s)'

Check warning on line 4712 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L4712

Added line #L4712 was not covered by tests
elif Aunits in {'m^5/(mol^2*s)', 'cm^5/(mol^2*s)', 'm^5/(molecule^2*s)', 'cm^5/(molecule^2*s)'}:
# surface: dissociative adsorption
Aunits = 'm^5/(mol^2*s)'

Check warning on line 4715 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L4715

Added line #L4715 was not covered by tests
elif Aunits == '':
# surface: sticking coefficient
pass

Check warning on line 4718 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L4718

Added line #L4718 was not covered by tests
else:
raise Exception(f'Invalid units {Aunits} for averaging kinetics.')

Check warning on line 4720 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L4720

Added line #L4720 was not covered by tests

if type(kinetics) not in [Arrhenius,]:
raise Exception(f'Invalid kinetics type {type(kinetics)!r} for {self!r}.')

Check warning on line 4723 in rmgpy/data/kinetics/family.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/kinetics/family.py#L4723

Added line #L4723 was not covered by tests

if False:
pass
else:
averaged_kinetics = Arrhenius(
A=(10 ** logA, Aunits),
n=n,
Ea=(Ea * 0.001, "kJ/mol"),
)
return averaged_kinetics
20 changes: 20 additions & 0 deletions rmgpy/data/kinetics/familyTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,15 @@
import numpy as np

from rmgpy import settings
import rmgpy.data.kinetics.family
from rmgpy.data.kinetics.database import KineticsDatabase
from rmgpy.data.kinetics.family import TemplateReaction
from rmgpy.data.rmg import RMGDatabase
from rmgpy.data.thermo import ThermoDatabase
from rmgpy.molecule import Molecule
from rmgpy.species import Species
from rmgpy.kinetics import Arrhenius



###################################################
Expand Down Expand Up @@ -1090,6 +1093,23 @@ def test_retaining_atom_labels_in_template_reaction(self):
self.assertEqual([(label, str(atom)) for label, atom in
reaction_list_2[0].labeled_atoms['products'].items()],
[('*1', 'C'), ('*2', 'C.'), ('*3', 'H')])

def test_average_kinetics(self):
"""
Test that the average kinetics are calculated correctly
"""
k1 = Arrhenius(A=(1e+13, 'cm^3/(mol*s)'), n=0, Ea=(0, 'kJ/mol'), T0=(1, 'K'))
k2 = Arrhenius(A=(4e+13, 'cm^3/(mol*s)'), n=1, Ea=(10, 'kJ/mol'), T0=(1, 'K'))
kav = rmgpy.data.kinetics.family.average_kinetics([k1, k2])
self.assertAlmostEqual(kav.A.value_si, 2.0e7, 2) # m3/mol/s
self.assertAlmostEqual(kav.n.value_si, 0.5, 6)
self.assertAlmostEqual(kav.Ea.value_si, 5.0e3, 2)
self.assertAlmostEqual(kav.T0.value_si, 1.0, 6)
self.assertEqual(kav.A.units, 'm^3/(mol*s)')
self.assertAlmostEqual(np.log(kav.get_rate_coefficient(300)),
np.average([np.log(k1.get_rate_coefficient(300)),
np.log(k2.get_rate_coefficient(300))]), 6)



################################################################################
Expand Down
3 changes: 3 additions & 0 deletions rmgpy/data/solvation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1198,6 +1198,9 @@
by gas-phase thermo estimate.
"""
molecule = species.molecule[0]
if molecule.contains_surface_site():
molecule = molecule.get_desorbed_molecules()[0]
molecule.saturate_unfilled_valence()

Check warning on line 1203 in rmgpy/data/solvation.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/data/solvation.py#L1202-L1203

Added lines #L1202 - L1203 were not covered by tests
molecule.clear_labeled_atoms()
molecule.update_atomtypes()
solute_data = self.estimate_solute_via_group_additivity(molecule)
Expand Down
1 change: 1 addition & 0 deletions rmgpy/rmg/input.py
Original file line number Diff line number Diff line change
Expand Up @@ -1374,6 +1374,7 @@ def generated_species_constraints(**kwargs):
'maximumSiliconAtoms',
'maximumSulfurAtoms',
'maximumSurfaceSites',
'maximumSurfaceBondOrder',
'maximumHeavyAtoms',
'maximumRadicalElectrons',
'maximumSingletCarbenes',
Expand Down
9 changes: 3 additions & 6 deletions rmgpy/rmg/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -1608,7 +1608,7 @@
kinetics=rxn.kinetics, duplicate=rxn.duplicate,
reversible=rxn.reversible
)
r, isNew = self.make_new_reaction(rxn) # updates self.new_species_list and self.newReactionlist
r, isNew = self.make_new_reaction(rxn) # updates self.new_species_list and self.new_reaction_list

Check warning on line 1611 in rmgpy/rmg/model.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/rmg/model.py#L1611

Added line #L1611 was not covered by tests
if getattr(r.kinetics, 'coverage_dependence', None):
self.process_coverage_dependence(r.kinetics)
if not isNew:
Expand Down Expand Up @@ -1712,8 +1712,8 @@
kinetics=rxn.kinetics, duplicate=rxn.duplicate,
reversible=rxn.reversible
)
r, isNew = self.make_new_reaction(rxn) # updates self.new_species_list and self.newReactionlist
if getattr(r.kinetics, 'coverage_dependence', None):
r, isNew = self.make_new_reaction(rxn) # updates self.new_species_list and self.new_reaction_list

Check warning on line 1715 in rmgpy/rmg/model.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/rmg/model.py#L1715

Added line #L1715 was not covered by tests
if r is not None and getattr(rxn.kinetics, 'coverage_dependence', None):
self.process_coverage_dependence(r.kinetics)
if not isNew:
logging.info("This library reaction was not new: {0}".format(rxn))
Expand Down Expand Up @@ -1760,9 +1760,6 @@
self.add_species_to_edge(spec)

for rxn in self.new_reaction_list:
# Note that we haven't actually evaluated any fluxes at this point
# Instead, we remove the comment below if the reaction is moved to
# the core later in the mechanism generation
if not (self.pressure_dependence and rxn.elementary_high_p and rxn.is_unimolecular()
and isinstance(rxn, LibraryReaction) and isinstance(rxn.kinetics, Arrhenius) and \
(self.pressure_dependence.maximum_atoms is None or self.pressure_dependence.maximum_atoms >= \
Expand Down
4 changes: 4 additions & 0 deletions rmgpy/rmg/reactors.py
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,10 @@ def __init__(self, core_phase_system, edge_phase_system, initial_conditions, ter

self.terminations = [to_rms(term) for term in terminations]

def get_const_spc_indices(self,spcs=None):
rms_species_names = [spc.name for spc in self.core_phase_system.get_rms_species_list()]
return [rms_species_names.index(name) for name in self.const_spc_names]

def finish_termination_criteria(self):
"""
Convert tuples into TerminationConversion objects
Expand Down
2 changes: 2 additions & 0 deletions rmgpy/yml.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@
result_dict["henrylawconstant"]["type"] = "TemperatureDependentHenryLawConstant"
result_dict["henrylawconstant"]["Ts"] = obj.henry_law_constant_data.Ts
result_dict["henrylawconstant"]["kHs"] = obj.henry_law_constant_data.kHs
result_dict["comment"] = obj.thermo.comment

Check warning on line 128 in rmgpy/yml.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/yml.py#L128

Added line #L128 was not covered by tests
elif isinstance(obj, NASA):
result_dict["polys"] = [obj_to_dict(k, spcs) for k in obj.polynomials]
result_dict["type"] = "NASA"
Expand All @@ -140,6 +141,7 @@
result_dict["type"] = "ElementaryReaction"
result_dict["radicalchange"] = sum([get_radicals(x) for x in obj.products]) - \
sum([get_radicals(x) for x in obj.reactants])
result_dict["comment"] = obj.kinetics.comment

Check warning on line 144 in rmgpy/yml.py

View check run for this annotation

Codecov / codecov/patch

rmgpy/yml.py#L144

Added line #L144 was not covered by tests
elif isinstance(obj, Arrhenius):
obj.change_t0(1.0)
result_dict["type"] = "Arrhenius"
Expand Down
Loading