Non-cat changes to master from cat branch, 1 #1538

rwest · 2019-01-21T04:41:08Z

Motivation or Problem

This is a collection of commits that prepares the groundwork for merging the cat branch (heterogeneous catalysis). I think they don't have anything to do with heterogenous catalysis.
My hope of collecting them in a separate pull request is that it will make code review easier.
We could split this PR further if desired.
I am hoping that these will not be too controversial or at least will not break anything.

Description of Changes

So far...

Fix a bug in the VF2 isomorphism code for disjoint graphs (will later be used for things physisorbed onto the surface)
Improve error logging in various places (implemented while debugging over the last 2.5 years)
Tweak a few docstrings and comments.
Edits to chemkin writing. Using a new function to convert units (because surface reactions can have very different dimensions, and simple heuristics will fail).

Testing

This now passes all the automated tests.

I believe the codecov report is not getting all the cythonized stuff.

codecov · 2019-01-22T03:06:04Z

Codecov Report

Merging #1538 into master will decrease coverage by 0.02%.
The diff coverage is 35.71%.

@@            Coverage Diff             @@
##           master    #1538      +/-   ##
==========================================
- Coverage   42.01%   41.99%   -0.03%     
==========================================
  Files         165      165              
  Lines       27873    27896      +23     
  Branches     5680     5680              
==========================================
+ Hits        11711    11715       +4     
- Misses      15377    15395      +18     
- Partials      785      786       +1

Impacted Files	Coverage Δ
rmgpy/molecule/atomtype.py	`0% <ø> (ø)`	⬆️
rmgpy/rmg/output.py	`53.36% <0%> (-0.28%)`	⬇️
rmgpy/quantity.py	`0% <0%> (ø)`	⬆️
rmgpy/data/thermo.py	`67.18% <100%> (ø)`	⬆️
rmgpy/data/kinetics/database.py	`46.86% <36.36%> (-0.57%)`	⬇️
rmgpy/molecule/draw.py	`42.67% <66.66%> (+0.69%)`	⬆️
rmgpy/data/kinetics/family.py	`58.29% <0%> (-0.3%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7e9d1c...e1f4e4b. Read the comment docs.

rwest · 2019-01-24T21:43:47Z

Running locally I can reproduce the continuous-integration/rmg-tests failure only when I run with the master branch of the RMG-database. When I run with the units branch (which has some corrections to the database), as I tried to configured in e074e0b, it works.

I.e. I think e074e0b, which attempted to follow the instructions at https://github.com/ReactionMechanismGenerator/RMG-Py/wiki/Simultaneous-Update-of-RMG-Py-and-RMG-database, does not work for the continuous-integration/rmg-tests (although it does work for the continuous-integration/travis-ci/pr and continuous-integration/travis-ci/push tests)

Perhaps once ReactionMechanismGenerator/RMG-database#311 is merged to RMG-database, these tests can be re-run, and this PR reviewed and merged.

rwest · 2019-01-25T14:05:38Z

The units fix has been merged to the master branch of RMG-database, so I've removed the commit that changed database branch, and am hoping this PR now passes the tests...

mliu49 · 2019-01-22T17:24:24Z

rmgpy/data/kinetics/database.py

+                    reaction_list.extend(family.generateReactions(molecules, products=products, prod_resonance=prod_resonance))
+                except:
+                    print("Problem family: {}".format(label))
+                    print("Problem reactants: {}".format(molecules))


Change to logging.error?

mliu49 · 2019-01-22T17:27:36Z

rmgpy/molecule/molecule.py

@@ -1001,6 +1001,18 @@ def getNumAtoms(self, element = None):
                    numAtoms += 1
            return numAtoms

+    def getNumberOfRadicalElectrons(self):


Duplicate of getRadicalCount?

Yes, you removed it from master in 60bbcfd but that got lost in a rebase, and it ended up being added again here (just by virtue of not being removed). Will remove again.

mliu49 · 2019-01-22T17:34:39Z

rmgpy/molecule/draw.py

+            self.coordinates[1, :] = [0.5, 0.0]
+            return self.coordinates
+
+        # Decide whether we can use RDKit or have to generate coordinates ourselves
        for atom in self.molecule.atoms:
            if atom.charge != 0:
                 flag_charge = 1


Should this be removed as well, since it is no longer used?

mliu49 · 2019-01-22T17:37:08Z

rmgpy/solver/simple.pyx

@@ -154,6 +154,7 @@ cdef class SimpleReactor(ReactionSystem):
            initialMoleFractions[speciesDict[label]] = moleFrac
        self.initialMoleFractions = initialMoleFractions

+


Extra line break.

rwest · 2019-01-25T22:02:37Z

I've made the changes suggested by @mliu49 and rebased.

mliu49 · 2019-01-29T16:21:47Z

@rwest, do you think you could also add the unit checking database test to the kinetics depositories tests? I found a unit mistake which was picked up by RMG-tests, and I expect there might be others as well.

R_Addition_MultipleBond/training

entry(
    index = 2,
    label = "CH2O + C3H5O <=> C4H7O2",
    degeneracy = 1.0,
    kinetics = Arrhenius(
        A = (54.3214, 's^-1', '*|/', 1.1507),
        n = 3.00879,
        Ea = (6.589, 'kcal/mol', '+|-', 0.024),
        T0 = (1, 'K'),
    ),
    referenceType = "theory",
    rank = 10,
    shortDesc = u"""nyee TST calculations at CBS-QB3 level with hindered rotors level""",
    longDesc = 
u"""
Quantum chemistry calculations at the CBS-QB3 level with hindered rotors
using Gaussian 03. High-pressure-limit rate coefficient computed
using Cantherm. One of the rotors had coupling and did not converge back to the
same initial geometry. It was forced to go back by editing the scan log.
""",
)

rwest · 2019-01-29T16:37:04Z

Good idea. Happy for you to add that if you have time before I do.

mliu49 · 2019-01-29T18:37:39Z

Ok, I can do that this afternoon.

mliu49 · 2019-01-29T20:26:44Z

I've added the unit test for kinetics depositories, which should cause Travis to fail until ReactionMechanismGenerator/RMG-database#312 is merged.

mliu49 · 2019-01-31T18:09:47Z

@rwest, could you rebase this branch? I think it's ready to merge.

I can also do it if you're busy, but I don't want to mess up the other cat branches.

… graphs This is what's currently causing us a problem in RMG-Cat for van-der-Waals bonds, but this is a minimal test case to reproduce the problem in the vf2 algorithm. Unsure at this stage if it's reasonable to fix the vf2 algorithm or if we'll have to work around it at a higher level.

If you have two graphs [1-2-3 4] and [1-2-3 4] then you can show they're isomorphic OK, but if you search for all the subgraph isomorphisms, it crashed. Now if there are no terminal atoms to choose from in, it chooses pairs only from non-matched vertices, instead of all vertices. I also added some commentary from the original paper.

…aster)

…ilies. Replace print() statements with logging.error() when loading bad reaction families.

Unless I misunderstood, I think this is clearer. (And if I did misunderstand, then it needs to be clearer!)

The idea is that if the object has units of 'm^3/mol/s' then but you want to get it in chemkin format you need the value in 'cm^3/mol/s'. This gives the factor required. Add a warning in docstring about potential bug.

This will allow us to do surface reactions with correct units. (Which will be implemented in a later commit) For gas-phase reactions, I put in a bunch of assert statements to check that the new implementation has the same effect as the old. If this is speed-critical code, they could be deprecated/removed.

Replaced the method getConversionFactorFromSItoCM which used to leave things like Molecules in their original form, with a new method getConversionFactorFromSItoCmMolS which converts everything into S (apart from length becomes cm). This means that all chemkin files are now written in CM/MOL/S combinations even if the original rate was in m3/molecules/minute. A few reaction libraries and training rules are stored in molecules instead of moles, but the chemkin file must be consistent.

When rotating by matrix algebra, we need to keep self.coondinates and coordinates pointing to the same numpy array, otherwise other things that reference or update one will not see or change the other. This commit just does the non-cat instance, so can be applied to master branch.

numSurfaceSpecies and numSurfaceReactions

These don't seem to be catalyst-specific. (Though will be helpful for ones that are)

Reduces the chances of passing the wrong thing by using positional arguments.

So far this does: - Arrhenius - Third Body - Falloff (Lindemann, Troe) - PDepArrhenius (PLOG) - MultiArrhenius - MultiPDepArrhenius in reaction libraries. This could also be done for other instances of kinetics (training depositories, etc.)

Make checkLibraryRateUnits more generic Add test definition for kinetics depositories

rwest force-pushed the catemp1 branch 2 times, most recently from 79caf96 to 08f6001 Compare January 22, 2019 02:31

rwest force-pushed the catemp1 branch from eaff7de to e074e0b Compare January 24, 2019 19:44

rwest force-pushed the catemp1 branch from e074e0b to f02722e Compare January 25, 2019 14:04

rwest mentioned this pull request Jan 25, 2019

Non-cat changes to master from cat branch, 2 #1542

Merged

mliu49 reviewed Jan 25, 2019

View reviewed changes

rwest added the Status: Ready for Review PR is complete and ready to be reviewed label Jan 25, 2019

rwest force-pushed the catemp1 branch 2 times, most recently from 66a901a to b307642 Compare January 25, 2019 22:02

rwest force-pushed the catemp1 branch from b307642 to c9ec8e5 Compare January 28, 2019 13:43

mliu49 mentioned this pull request Jan 29, 2019

Correct A factor units in training depositories ReactionMechanismGenerator/RMG-database#312

Merged

rwest added 10 commits January 31, 2019 14:45

More honest error logging when screwing up drawing.

7b5e828

Increase error logging specificity when loading reaction families. (m…

ea502be

…aster)

Better error logging in thermo database descending tree

43d94c7

Improved error logging in Kinetics database when loading reaction fam…

686f300

…ilies. Replace print() statements with logging.error() when loading bad reaction families.

Fix a docstring in NASA Polynomical class

37b8370

Fix tiny typos in some comments.

59cee5c

Move where a comment line is, I think to clarify.

8b499d1

Unless I misunderstood, I think this is clearer. (And if I did misunderstand, then it needs to be clearer!)

Reduce code duplication in saveChemkin function. (master)

8e839e2

rwest and others added 11 commits January 31, 2019 15:08

Create a getConversionFactorFromSItoCM method on Units.

b9145c6

The idea is that if the object has units of 'm^3/mol/s' then but you want to get it in chemkin format you need the value in 'cm^3/mol/s'. This gives the factor required. Add a warning in docstring about potential bug.

Adding getConversionFactorFromSItoCM() unit tests

5c7c183

Setup.py removes duplicate extension modules.

1c1eb8c

Add cdef type declarations for base solver class

f86e5d6

numSurfaceSpecies and numSurfaceReactions

Changes to molecule/draw.py

ad155a6

These don't seem to be catalyst-specific. (Though will be helpful for ones that are)

Force simpleReactor initialization to use keyword arguments.

bdec0ff

Reduces the chances of passing the wrong thing by using positional arguments.

Add units check for kinetics depositories to database test

e1f4e4b

Make checkLibraryRateUnits more generic Add test definition for kinetics depositories

rwest force-pushed the catemp1 branch from cce2a3c to e1f4e4b Compare January 31, 2019 20:08

mliu49 approved these changes Jan 31, 2019

View reviewed changes

mliu49 merged commit c21e3e5 into master Jan 31, 2019

mliu49 deleted the catemp1 branch January 31, 2019 22:16

mjohnson541 mentioned this pull request Feb 1, 2019

Automatic Tree Generation Part 3 #1461

Merged

rwest removed the Status: Ready for Review PR is complete and ready to be reviewed label Feb 1, 2019

mliu49 mentioned this pull request May 15, 2019

RMG v2.4.0 (?) Release Planning #1591

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-cat changes to master from cat branch, 1 #1538

Non-cat changes to master from cat branch, 1 #1538

rwest commented Jan 21, 2019 •

edited

Loading

codecov bot commented Jan 22, 2019 •

edited

Loading

rwest commented Jan 24, 2019

rwest commented Jan 25, 2019

mliu49 Jan 22, 2019

mliu49 Jan 22, 2019

rwest Jan 25, 2019

mliu49 Jan 22, 2019

mliu49 Jan 22, 2019

rwest commented Jan 25, 2019

mliu49 commented Jan 29, 2019

rwest commented Jan 29, 2019

mliu49 commented Jan 29, 2019

mliu49 commented Jan 29, 2019

mliu49 commented Jan 31, 2019

		@@ -154,6 +154,7 @@ cdef class SimpleReactor(ReactionSystem):
		initialMoleFractions[speciesDict[label]] = moleFrac
		self.initialMoleFractions = initialMoleFractions

Non-cat changes to master from cat branch, 1 #1538

Non-cat changes to master from cat branch, 1 #1538

Conversation

rwest commented Jan 21, 2019 • edited Loading

Motivation or Problem

Description of Changes

Testing

codecov bot commented Jan 22, 2019 • edited Loading

Codecov Report

rwest commented Jan 24, 2019

rwest commented Jan 25, 2019

mliu49 Jan 22, 2019

Choose a reason for hiding this comment

mliu49 Jan 22, 2019

Choose a reason for hiding this comment

rwest Jan 25, 2019

Choose a reason for hiding this comment

mliu49 Jan 22, 2019

Choose a reason for hiding this comment

mliu49 Jan 22, 2019

Choose a reason for hiding this comment

rwest commented Jan 25, 2019

mliu49 commented Jan 29, 2019

rwest commented Jan 29, 2019

mliu49 commented Jan 29, 2019

mliu49 commented Jan 29, 2019

mliu49 commented Jan 31, 2019

rwest commented Jan 21, 2019 •

edited

Loading

codecov bot commented Jan 22, 2019 •

edited

Loading