Conversation
amarkpayne
force-pushed
the
isodesmic_improvements
branch
from
f719108 to
038bca4
Compare
|
This pull request introduces 1 alert when merging 038bca4 into 47c4e0b - view on LGTM.com new alerts:
|
|
At a first look, this PR looks good. As minor changes:
|
davidfarinajr
left a comment
davidfarinajr
left a comment
There was a problem hiding this comment.
Thanks for these improvements @amarkpayne ! Overall, it is looking very good. Nice work! I did find one bug for generating allowable_charges for anions (see comments). Also, I see that there are 2 unit tests that are failing. I though that they are failing because of the new limit_scope feature which default setting is True. However, it turns out that this is not the case. I need to do a little more digging to see why these tests are failing
arkane/encorr/isodesmic.py
Outdated
| connections = [] | ||
| for a, b in atom.bonds.items(): | ||
| ac = AtomConstraint(label=f'{a.symbol}{a.connectivity1}') | ||
| bond_order = b.order |
There was a problem hiding this comment.
Should we use int(bond.order) here (which we are using above in _buerger_rc2 and _buerger_rc3)?
There was a problem hiding this comment.
Thanks for the catch!
arkane/encorr/isodesmic.py
Outdated
| new_constraints = [] | ||
|
|
||
| if charge < 0: | ||
| allowable_charges = list(range(charge, 0)) |
There was a problem hiding this comment.
If we want to include neutral charge species, we should change this to allowable_charges = list(range(charge, 1))
There was a problem hiding this comment.
Yes, thanks for the catch!
amarkpayne
left a comment
amarkpayne
left a comment
There was a problem hiding this comment.
Thanks for the comments @davidfarinajr ! I finally got around to these fixes, and I think this should be good to go now. Let me know what you think
arkane/encorr/isodesmic.py
Outdated
| connections = [] | ||
| for a, b in atom.bonds.items(): | ||
| ac = AtomConstraint(label=f'{a.symbol}{a.connectivity1}') | ||
| bond_order = b.order |
There was a problem hiding this comment.
Thanks for the catch!
arkane/encorr/isodesmic.py
Outdated
| new_constraints = [] | ||
|
|
||
| if charge < 0: | ||
| allowable_charges = list(range(charge, 0)) |
There was a problem hiding this comment.
Yes, thanks for the catch!
amarkpayne
force-pushed
the
isodesmic_improvements
branch
from
038bca4 to
0ab32e4
Compare
Adding charge as a constraint broke pyomo, but it is unclear how. lpsolve works fine. This commit is needed until a fix is implemented
Each time a new reaction is found, eliminate subsets from the queue that contain all the indices of every species in that reaction. If such a subset was run, it would be possible to arrive at the same reaction.
It is not needed, as we always do this before solving, and it can lead to an error if the constraint matrix is not solvable (this method return None to signal this)
Every subset added is a strict subset of the ones that came before it. Therefore, it is redundant to keep checking previously found reactions
Many of the charged species throw warnings, but they are actually being handled properly
Our code is able to tell when pyomo fails and deals with it properly
For consistent behavior with pyomo
This actually seems to increase performance and saves time
We should only test if the thermo is within a reasonable range, and that more than one reaction is found
amarkpayne
force-pushed
the
isodesmic_improvements
branch
from
0ab32e4 to
4259d1e
Compare
Codecov Report
@@ 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
... and 50 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
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. |
|
@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 thanks for the update! I am going to resolve the conflicts in the |
|
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
added
the
abandoned
|
In the time since this PR was opened, the new |
JacksonBurns
removed
stale
4 participants
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.