RMG-Py icon indicating copy to clipboard operation
RMG-Py copied to clipboard

Isodesmic improvements

Open amarkpayne opened this issue 3 years ago • 3 comments

Motivation or Problem

There are a few improvements I made to the isodesmic reactions code in RMG as part of the work on an upcoming BACs paper. With these improvements, isodesmic reactions show similar performance to BACs under certain conditions.

Description of Changes

The biggest change is adding in higher order isodesmic schemes

Testing

I used this branch to generate data/plots for the upcomming BACs paper. I can share a talk on this to help with the review.

amarkpayne avatar Oct 18 '21 12:10 amarkpayne

This pull request introduces 1 alert when merging 038bca4152d16dab8063edcca52774b5a9d3947f into 47c4e0b16ee274919ec24aa4160ac5e83cc9f0a9 - view on LGTM.com

new alerts:

  • 1 for Unused import

lgtm-com[bot] avatar Oct 18 '21 13:10 lgtm-com[bot]

At a first look, this PR looks good. As minor changes:

  • Probably squash these two commits that both address pyomo? (commit 1 commit 2 )

  • Address the failing unit test due to an assert error https://github.com/ReactionMechanismGenerator/RMG-Py/pull/2217/checks?check_run_id=3926584877#step:10:3823

  • Fix the minor LGTM complaint about an unused LE import?

kspieks avatar Oct 18 '21 15:10 kspieks

Codecov Report

Merging #2217 (4259d1e) into main (1d67d94) will decrease coverage by 0.82%. The diff coverage is 83.50%.

:exclamation: Current head 4259d1e differs from pull request most recent head 5e8919b. Consider uploading reports for the commit 5e8919b to get more accurate results

@@            Coverage Diff             @@
##             main    #2217      +/-   ##
==========================================
- Coverage   48.49%   47.67%   -0.82%     
==========================================
  Files         110      105       -5     
  Lines       30758    28275    -2483     
  Branches     8039     7479     -560     
==========================================
- Hits        14917    13481    -1436     
+ Misses      14318    13343     -975     
+ Partials     1523     1451      -72     
Impacted Files Coverage Δ
arkane/encorr/isodesmic.py 79.44% <82.81%> (-0.98%) :arrow_down:
arkane/encorr/reference.py 71.32% <100.00%> (+0.86%) :arrow_up:

... and 50 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Mar 02 '22 01:03 codecov[bot]

This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days.

github-actions[bot] avatar Jun 21 '23 22:06 github-actions[bot]

@JacksonBurns this PR should make its way into RMG at some point. If you want I can take one last look at it, update it to main, and then merge it in. Also happy to wait for approval if someone on the team wants to take over as reviewer. @oscarwumit is working on the manuscript for this, so he might be the most familiar person there.

amarkpayne avatar Jun 22 '23 00:06 amarkpayne

@amarkpayne thanks for the update! I am going to resolve the conflicts in the environment.yml file and see if this is still close to merging. Please give it another look once the CI gets underway.

JacksonBurns avatar Jun 22 '23 14:06 JacksonBurns

This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days.

github-actions[bot] avatar Sep 21 '23 08:09 github-actions[bot]

In the time since this PR was opened, the new pyutillib dependency has ceased development. Refactoring this PR to deal with this is currently not in the cards, so I'm going to remove the stale and abandoned labels and soft reject this PR.

JacksonBurns avatar Oct 22 '23 20:10 JacksonBurns