Collection of small fixes by mliu49 · Pull Request #1842 · ReactionMechanismGenerator/RMG-Py

mliu49 · 2019-12-04T20:45:46Z

Motivation or Problem

This PR addresses a number of minor issues in preparation for the 3.0 release.

Description of Changes

Please see the individual commit messages for the changes.

Testing

I think passing unit tests should be sufficient since these are all relatively minor.

Reviewer Tips

Thoughts on the approaches chosen here would be nice.

I think the increased tolerance for the pdep test may be worth additional discussion. There are still some ongoing efforts to try and identify the root cause of that issue.

mliu49 · 2019-12-04T21:03:51Z

Changing the tolerance for the pdep test worked for that assertion, but the numerical differences also carry through to the sensitivity analysis, which is causing the test to fail.

alongd · 2019-12-05T15:32:06Z

rmgpy/molecule/translator.py

            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()))


Maybe instead add the adjlist to the error message below?

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.

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

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

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.

codecov · 2019-12-06T16:27:49Z

Codecov Report

Merging #1842 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1842      +/-   ##
==========================================
+ Coverage   43.91%   43.96%   +0.05%     
==========================================
  Files          83       83              
  Lines       21564    21518      -46     
  Branches     5652     5639      -13     
==========================================
- Hits         9469     9461       -8     
+ Misses      11048    10996      -52     
- Partials     1047     1061      +14

Impacted Files	Coverage Δ
rmgpy/thermo/thermoengine.py	`80.88% <ø> (-1.56%)`	⬇️
rmgpy/data/kinetics/family.py	`49.06% <ø> (+0.7%)`	⬆️
arkane/kinetics.py	`12.14% <0%> (ø)`	⬆️
rmgpy/data/statmech.py	`42.2% <0%> (ø)`	⬆️
rmgpy/rmg/pdep.py	`12.21% <0%> (ø)`	⬆️
rmgpy/statmech/ndTorsions.py	`59.78% <0%> (ø)`	⬆️
rmgpy/yml.py	`15.71% <0%> (ø)`	⬆️
arkane/sensitivity.py	`10% <0%> (ø)`	⬆️
rmgpy/data/kinetics/database.py	`50.61% <0%> (ø)`	⬆️

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 9ea90fe...8748f73. Read the comment docs.

amarkpayne

A couple of quick comments. Add your fix for the pdep test, rebase, and I'll merge.

amarkpayne · 2019-12-06T19:29:43Z

rmgpy/data/kinetics/family.py

-        and wb (total bond energy of bonds broken) with w0 = (wf+wb)/2
-        """
-        mol = None
-        a_dict = {}


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

amarkpayne · 2019-12-06T19:35:23Z

rmgpy/data/kinetics/family.py

            for species in reaction.reactants + reaction.products:
                species.generate_resonance_structures()

-    def get_w0(self, rxn):


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

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.

amarkpayne · 2019-12-06T19:41:48Z

rmgpy/thermo/thermoengine.py

-            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))
+    # if thermo.__class__ != thermo0.__class__:


I know it is better to comment out so that it is easy to find in case anyone else ever needs it, but calculating the error in the fit is fairly straight forward, so if this is something we decide to implement in the future it won't take long to write from scratch, and the resulting implementation might be better for it. I think we can delete this code altogether, but if you prefer it can stay commented out

I removed the code. You're right that it would be easy to rewrite if ever needed.

amarkpayne · 2019-12-06T19:53:29Z

arkane/pdepTest.py

-        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)


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)

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.

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

Likely left over from move to kinetics.arrhenius Resolves #1826

We were not doing anything with the results, so it was doing pointless computations. Resolves #1812

This makes it easier to figure out the problematic molecule

This enables identifier generation for surface species using OpenBabel as the backend Copies the approach used for RDKit by replacing X with Pt

mliu49 · 2019-12-06T20:18:15Z

There is one new commit replacing the mock external library with unittest.mock.

Resolves #1682

mock was added into the standard library in Python 3.3

mliu49 added Complexity: Low Good First Issue For Next Release labels Dec 4, 2019

mliu49 requested review from alongd, kspieks and mjohnson541 December 4, 2019 20:45

mliu49 self-assigned this Dec 4, 2019

alongd reviewed Dec 5, 2019

View reviewed changes

Add more line delimiters to chemkin._remove_line_breaks

2899833

mliu49 force-pushed the fixes branch from 7f5f3d4 to e94b327 Compare December 6, 2019 15:56

amarkpayne self-requested a review December 6, 2019 18:56

amarkpayne reviewed Dec 6, 2019

View reviewed changes

mliu49 added 5 commits December 6, 2019 15:05

Remove duplicate get_w0 and get_w0s methods in KineticsFamily

71abbcb

Likely left over from move to kinetics.arrhenius Resolves #1826

Do not calculate error from thermo model conversion

e926c06

We were not doing anything with the results, so it was doing pointless computations. Resolves #1812

Add logging of molecule adjlist when identifier generation fails

7d2e60c

This makes it easier to figure out the problematic molecule

Add support for converting surface species to obmol

987cf74

This enables identifier generation for surface species using OpenBabel as the backend Copies the approach used for RDKit by replacing X with Pt

Change classmethod args from self to cls

1655008

mliu49 force-pushed the fixes branch from 0e9f748 to 3ede256 Compare December 6, 2019 20:05

mliu49 added 3 commits December 6, 2019 15:40

Increase tolerance for rate check in pdep test

08e8a1d

Resolves #1682

Replace mock with unittest.mock

58fce35

mock was added into the standard library in Python 3.3

Minor PEP-8 fixes to arrhenius.get_w0

8748f73

mliu49 force-pushed the fixes branch from 3ede256 to 8748f73 Compare December 6, 2019 20:40

amarkpayne approved these changes Dec 6, 2019

View reviewed changes

kspieks mentioned this pull request Dec 6, 2019

Allow mole fractions to be normalized if they do not sum to one #1809

Merged

amarkpayne merged commit d4f77cd into master Dec 6, 2019

amarkpayne deleted the fixes branch December 6, 2019 22:46

amarkpayne mentioned this pull request Dec 6, 2019

Extra redundant line in chem_annotated.inp file caused ChemkinError #1837

Closed

This was referenced Dec 13, 2019

RMG v3.0.0 Release Planning #1830

Closed

RMG-Py v3.0.0 Release #1852

Merged

Conversation

mliu49 commented Dec 4, 2019

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

Uh oh!

mliu49 commented Dec 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amarkpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mliu49 commented Dec 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 6, 2019 •

edited

Loading