Skip to content

skip generation when # tree reactants != # given reactants#1675

Merged
mliu49 merged 3 commits intomasterfrom
atg_fix
Aug 7, 2019
Merged

skip generation when # tree reactants != # given reactants#1675
mliu49 merged 3 commits intomasterfrom
atg_fix

Conversation

@mjohnson541
Copy link
Copy Markdown
Contributor

@mjohnson541 mjohnson541 commented Aug 7, 2019

Fixes #1674.

@mjohnson541 mjohnson541 requested a review from mliu49 August 7, 2019 15:15
@alongd
Copy link
Copy Markdown
Member

alongd commented Aug 7, 2019

Could you also add a test for a birad?

@amarkpayne amarkpayne added Before Py3 Should be merged before Python 3 transition Priority: Critical Should be addressed ASAP Type: Bug labels Aug 7, 2019
mliu49
mliu49 reviewed Aug 7, 2019
View reviewed changes
rmgpy/data/kinetics/family.py

reactantNum = self.productNum

if self.autoGenerated and reactantNum != len(reactants):
Copy link
Copy Markdown
Contributor

@mliu49 mliu49 Aug 7, 2019

Choose a reason for hiding this comment

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

Does it have to be autoGenerated for this to be a good check to do?

Copy link
Copy Markdown
Contributor Author

@mjohnson541 mjohnson541 Aug 7, 2019

Choose a reason for hiding this comment

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

Yes, because for non-autogenerated trees self.reactantNum seems to be guessed from the template number, which really only works for bimolecular families.

@mliu49
Copy link
Copy Markdown
Contributor

mliu49 commented Aug 7, 2019

I think this looks fine overall. I agree that it would be good to add a test.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5f95c4d). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1675   +/-   ##
=========================================
  Coverage          ?   41.55%           
=========================================
  Files             ?      176           
  Lines             ?    29837           
  Branches          ?     6192           
=========================================
  Hits              ?    12399           
  Misses            ?    16530           
  Partials          ?      908
Impacted Files Coverage Δ
rmgpy/data/kinetics/family.py 48.87% <100%> (ø)

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 5f95c4d...b7e4fac. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 7, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@5f95c4d). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1675   +/-   ##
=========================================
  Coverage          ?   41.54%           
=========================================
  Files             ?      176           
  Lines             ?    29840           
  Branches          ?     6192           
=========================================
  Hits              ?    12398           
  Misses            ?    16533           
  Partials          ?      909
Impacted Files Coverage Δ
...tabase/kinetics/families/R_Recombination/groups.py 100% <100%> (ø)
rmgpy/data/kinetics/family.py 48.71% <100%> (ø)

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 5f95c4d...212a224. Read the comment docs.

mliu49
mliu49 reviewed Aug 7, 2019
View reviewed changes
rmgpy/data/kinetics/familyTest.py


def testReactantNumID(self):
family = self.database.families['R_Recombination']
Copy link
Copy Markdown
Contributor

@mliu49 mliu49 Aug 7, 2019

Choose a reason for hiding this comment

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

Could you add a docstring?

Copy link
Copy Markdown
Contributor Author

@mjohnson541 mjohnson541 Aug 7, 2019

Choose a reason for hiding this comment

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

fixed

rmgpy/data/kinetics/familyTest.py Outdated
spc = Molecule().fromSMILES("[CH2]CC[CH2]")
out = family._KineticsFamily__generateReactions(reactants=[spc],forward=True)
logging.error(out)
self.assertTrue(out==[])
Copy link
Copy Markdown
Contributor

@mliu49 mliu49 Aug 7, 2019

Choose a reason for hiding this comment

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

You can use assertEqual.

Copy link
Copy Markdown
Contributor Author

@mjohnson541 mjohnson541 Aug 7, 2019

Choose a reason for hiding this comment

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

done

rmgpy/data/kinetics/familyTest.py Outdated
family = self.database.families['R_Recombination']
spc = Molecule().fromSMILES("[CH2]CC[CH2]")
out = family._KineticsFamily__generateReactions(reactants=[spc],forward=True)
logging.error(out)
Copy link
Copy Markdown
Contributor

@mliu49 mliu49 Aug 7, 2019

Choose a reason for hiding this comment

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

I don't think you need to log this. If the assertion fails, it will print out the value.

Copy link
Copy Markdown
Contributor Author

@mjohnson541 mjohnson541 Aug 7, 2019

Choose a reason for hiding this comment

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

done

@mliu49 mliu49 merged commit 114bd34 into master Aug 7, 2019
@mliu49 mliu49 deleted the atg_fix branch August 7, 2019 21:12
@mliu49 mliu49 mentioned this pull request Nov 26, 2019
Closed
@mliu49 mliu49 mentioned this pull request Dec 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mliu49 mliu49 mliu49 approved these changes

Assignees

No one assigned

Labels

Before Py3 Should be merged before Python 3 transition Priority: Critical Should be addressed ASAP Type: Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Minimal Example Does Not Run on Current Master

4 participants

@mjohnson541 @alongd @mliu49 @amarkpayne