Skip to content
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

Use autoGenerated tag instead of the hardcoded family list #2157

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

hwpang
Copy link
Contributor

@hwpang hwpang commented Jun 14, 2021

Motivation or Problem

Multiple PRs in RMG-database related to tree generation failed the database test (e.g. ReactionMechanismGenerator/RMG-database#493 and ReactionMechanismGenerator/RMG-database#495)

Description of Changes

Made the code to look for auto_generated tag instead of the hardcoded list to avoid needing to twin PR

Testing

I didn't run a test as this should be a straight forward change

@hwpang hwpang requested review from mjohnson541 and ChrisBNEU June 14, 2021 20:20
Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

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

Good idea. Made a couple of suggestions.

testing/databaseTest.py Outdated Show resolved Hide resolved
testing/databaseTest.py Outdated Show resolved Hide resolved
@hwpang hwpang force-pushed the fix_databaseTestforTreeGen branch from 8ca9754 to 3fc42a1 Compare June 14, 2021 20:30
@hwpang
Copy link
Contributor Author

hwpang commented Jun 14, 2021

@rwest Thanks for the catch!

rwest
rwest previously approved these changes Jun 14, 2021
Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

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

Now looks good to me, but I'd wait for the automated tests before merging, to be safe.

ChrisBNEU
ChrisBNEU previously approved these changes Jun 14, 2021
Copy link
Contributor

@ChrisBNEU ChrisBNEU left a comment

Choose a reason for hiding this comment

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

looks good to me as well. thanks!

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #2157 (3fc42a1) into master (98a7d80) will increase coverage by 0.34%.
The diff coverage is n/a.

❗ Current head 3fc42a1 differs from pull request most recent head bf8afcd. Consider uploading reports for the commit bf8afcd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2157      +/-   ##
==========================================
+ Coverage   47.91%   48.26%   +0.34%     
==========================================
  Files         101      101              
  Lines       26989    27210     +221     
  Branches     6911     7125     +214     
==========================================
+ Hits        12932    13133     +201     
+ Misses      12684    12680       -4     
- Partials     1373     1397      +24     
Impacted Files Coverage Δ
arkane/kinetics.py 12.24% <0.00%> (ø)
rmgpy/molecule/draw.py 53.26% <0.00%> (+1.28%) ⬆️
rmgpy/data/kinetics/family.py 50.24% <0.00%> (+2.79%) ⬆️

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 98a7d80...bf8afcd. Read the comment docs.

@mjohnson541
Copy link
Contributor

Can you rebase?

@hwpang hwpang dismissed stale reviews from ChrisBNEU and rwest via bf8afcd June 15, 2021 00:36
@hwpang hwpang force-pushed the fix_databaseTestforTreeGen branch from 3fc42a1 to bf8afcd Compare June 15, 2021 00:36
@hwpang
Copy link
Contributor Author

hwpang commented Jun 15, 2021

Can you rebase?

Rebased!

@mjohnson541 mjohnson541 merged commit a649393 into master Jun 15, 2021
@mjohnson541 mjohnson541 deleted the fix_databaseTestforTreeGen branch June 15, 2021 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants