Reverse recipes should be in reverse order #1829

mazeau · 2019-11-23T22:39:44Z

This is a duplicate of PR #1827, but only contains the relevant commits.

I found that when generating reverse templates, the action list would go in the same order as the forward, and this would cause issues when making and breaking multiple bonds.

    ['CHANGE_BOND', '*1', 1, '*2'],
    ['CHANGE_BOND', '*2', -1, '*3'],
    ['BREAK_BOND', '*2', 1, '*3'],
    ['FORM_BOND', '*3', 1, '*4'],
    ['CHANGE_BOND', '*3', 1, '*4']

This change fixes this template's reverse recipe.

mliu49 · 2019-11-25T17:09:38Z

rmgpy/data/kinetics/family.py

@@ -258,6 +258,7 @@ def get_reverse(self):
                other.add_action(['GAIN_PAIR', action[1], action[2]])
            elif action[0] == 'GAIN_PAIR':
                other.add_action(['LOSE_PAIR', action[1], action[2]])
+        other.actions = reversed(other.actions) # Play the reverse recipe in the reverse order


reversed returns an iterator. Do we expect any issues with that?

I didn't think that would cause any issues. I'm able to run it perfectly fine. Maybe @rwest feels differently?

Hard to anticipate, but might as well try other.actions = list(reversed(other.actions)).

Then should (rebase and) look into why the unit tests failed.

Actually, I have a better suggestion: use reversed directly on the for loop, for action in reversed(self.actions)

mliu49 · 2019-11-26T16:23:56Z

The failing unit tests are all related to reaction generation:

======================================================================
ERROR: test_reaction_degeneracy_independent_of_generatereactions_direction
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/ReactionMechanismGenerator/RMG-Py/rmgpy/data/kinetics/kineticsTest.py", line 565, in test_reaction_degeneracy_independent_of_generatereactions_direction
    self.assertEqual(forward_reactions[0].degeneracy, reverse_reactions[0].degeneracy,
IndexError: list index out of range
======================================================================
FAIL: Test that the ``react_all`` function works in serial
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/ReactionMechanismGenerator/RMG-Py/rmgpy/rmg/reactTest.py", line 124, in test_react_all
    self.assertEqual(len(flat_rxn_list), 44)
AssertionError: 25 != 44
======================================================================
FAIL: Test that the ``react_all`` function works in parallel using Python multiprocessing
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/ReactionMechanismGenerator/RMG-Py/rmgpy/rmg/reactTest.py", line 149, in test_react_all_parallel
    self.assertEqual(len(flat_rxn_list), 44)
AssertionError: 25 != 44

I don't have any intuition for why this change would have these effects, so you would need to do some debugging.

codecov · 2019-12-03T18:29:56Z

Codecov Report

Merging #1829 into master will decrease coverage by 1.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1829      +/-   ##
==========================================
- Coverage   43.98%   42.89%   -1.09%     
==========================================
  Files          83       83              
  Lines       21581    21565      -16     
  Branches     5653     5652       -1     
==========================================
- Hits         9492     9251     -241     
- Misses      11043    11290     +247     
+ Partials     1046     1024      -22

Impacted Files	Coverage Δ
rmgpy/data/kinetics/family.py	`48.35% <100%> (ø)`	⬆️
rmgpy/qm/molecule.py	`24.77% <0%> (-55.41%)`	⬇️
rmgpy/qm/mopac.py	`27.05% <0%> (-42.95%)`	⬇️
rmgpy/qm/qmdata.py	`55.88% <0%> (-29.42%)`	⬇️
rmgpy/qm/main.py	`50% <0%> (-15.33%)`	⬇️
rmgpy/data/solvation.py	`63.78% <0%> (-1.34%)`	⬇️

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 a87d79f...de77c4b. Read the comment docs.

Recipes that decrement THEN break a bond, when reversed must create THEN increment the bond, not increment then create. i.e. the recipe steps must be played backwards in order. reversing the loop, suggested by mliu49.

rwest · 2019-12-03T22:08:30Z

I can't imagine why this change would cause a

======================================================================
5124FAIL: A general test for a PDep job in Arkane
5125----------------------------------------------------------------------
5126Traceback (most recent call last):
5127  File "/home/travis/build/ReactionMechanismGenerator/RMG-Py/arkane/pdepTest.py", line 107, in test_pdep_job
5128    self.assertAlmostEquals(rxn.kinetics.get_rate_coefficient(1000.0, 1.0), 88.88253229631246)
5129AssertionError: 88.9961408998604 != 88.88253229631246 within 7 places (0.11360860354794511 difference)

that was showing up in Travis, so I've triggered a new travis build, in case it's something weird or random.

mliu49 · 2019-12-03T22:12:25Z

It seems that a lot of new builds are failing due to that. It's discussed somewhat in #1682, but we were not able to reach a conclusion. It is possible that this new wave of failures is caused by a new version of some dependency, but the idea that math could change (by 0.1!) is quite odd.

mliu49

Thanks for the contribution!

mliu49 reviewed Nov 25, 2019

View reviewed changes

mazeau force-pushed the recipe branch from 91a80df to 95dc1ed Compare December 3, 2019 19:18

Reverses the recipe actions for reverse templates

de77c4b

Recipes that decrement THEN break a bond, when reversed must create THEN increment the bond, not increment then create. i.e. the recipe steps must be played backwards in order. reversing the loop, suggested by mliu49.

mazeau force-pushed the recipe branch from 570749a to de77c4b Compare December 3, 2019 19:40

mliu49 mentioned this pull request Dec 3, 2019

Arkane Pdep Functional Test Fails Often #1682

Closed

mliu49 approved these changes Dec 4, 2019

View reviewed changes

mliu49 merged commit b691b45 into ReactionMechanismGenerator:master Dec 4, 2019

mliu49 mentioned this pull request Dec 4, 2019

RMG v3.0.0 Release Planning #1830

Closed

mazeau deleted the recipe branch December 4, 2019 18:52

mliu49 mentioned this pull request Dec 16, 2019

RMG-Py v3.0.0 Release #1852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reverse recipes should be in reverse order #1829

Reverse recipes should be in reverse order #1829

mazeau commented Nov 23, 2019

mliu49 Nov 25, 2019

mazeau Nov 25, 2019

rwest Nov 25, 2019

mliu49 Nov 26, 2019

mliu49 commented Nov 26, 2019

codecov bot commented Dec 3, 2019 •

edited

Loading

rwest commented Dec 3, 2019

mliu49 commented Dec 3, 2019

mliu49 left a comment

Reverse recipes should be in reverse order #1829

Reverse recipes should be in reverse order #1829

Conversation

mazeau commented Nov 23, 2019

mliu49 Nov 25, 2019

Choose a reason for hiding this comment

mazeau Nov 25, 2019

Choose a reason for hiding this comment

rwest Nov 25, 2019

Choose a reason for hiding this comment

mliu49 Nov 26, 2019

Choose a reason for hiding this comment

mliu49 commented Nov 26, 2019

codecov bot commented Dec 3, 2019 • edited Loading

Codecov Report

rwest commented Dec 3, 2019

mliu49 commented Dec 3, 2019

mliu49 left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2019 •

edited

Loading