-
Notifications
You must be signed in to change notification settings - Fork 230
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
skip generation when # tree reactants != # given reactants #1675
Conversation
Could you also add a test for a birad? |
|
||
reactantNum = self.productNum | ||
|
||
if self.autoGenerated and reactantNum != len(reactants): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I think this looks fine overall. I agree that it would be good to add a test. |
Codecov Report
@@ Coverage Diff @@
## master #1675 +/- ##
=========================================
Coverage ? 41.55%
=========================================
Files ? 176
Lines ? 29837
Branches ? 6192
=========================================
Hits ? 12399
Misses ? 16530
Partials ? 908
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1675 +/- ##
=========================================
Coverage ? 41.54%
=========================================
Files ? 176
Lines ? 29840
Branches ? 6192
=========================================
Hits ? 12398
Misses ? 16533
Partials ? 909
Continue to review full report at Codecov.
|
|
||
|
||
def testReactantNumID(self): | ||
family = self.database.families['R_Recombination'] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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==[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use assertEqual
.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Fixes #1674.