Cat fixes - vdw radicals, bidentates, resonance #2136

davidfarinajr · 2021-05-21T00:29:15Z

Motivation or Problem and Description of Changes

vdw radicals - We previously implemented a multiplicity check to ensure products in the reverse direction match the multiplicity of the template reactants . However, we are getting vdw products that do not obey multiplicity restriction in the forward direction (Undeterminable Kinetics Error of Surface_Abstraction_vdW family #2122) , so we also need to check this direction as well.
radicals on X - if resonance generation puts radicals/lone pairs/or a formal charge on X, RMG crashes. I added a commit on this PR to filter out these structures
bidentate vdw - We were getting bidentate vdw structures. This PR makes them forbidden for surface families

Testing

added unit test for forbidden vdw bidentates and successfully built an ammonia cat oxidation model that was previously crashing.

codecov · 2021-05-21T01:24:39Z

Codecov Report

Merging #2136 (08c6343) into master (e414514) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #2136      +/-   ##
==========================================
- Coverage   47.24%   47.23%   -0.01%     
==========================================
  Files          89       89              
  Lines       24151    24169      +18     
  Branches     6302     6309       +7     
==========================================
+ Hits        11409    11417       +8     
- Misses      11531    11539       +8     
- Partials     1211     1213       +2

Impacted Files	Coverage Δ
rmgpy/data/kinetics/family.py	`47.27% <50.00%> (-0.02%)`	⬇️

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 e414514...08c6343. Read the comment docs.

mazeau

I think everything here looks fine. There definitely shouldn't be any bidentate things matching with vdw families.

I don't think the best thing to do is to trash species after we generate charges/radicals/lone pairs on surface sites, but to prevent rmg from suggesting them in the first place.

davidfarinajr · 2021-05-21T19:21:37Z

I think everything here looks fine. There definitely shouldn't be any bidentate things matching with vdw families.

I don't think the best thing to do is to trash species after we generate charges/radicals/lone pairs on surface sites, but to prevent rmg from suggesting them in the first place.

Yes, I agree. The fix on this PR is a band-aid to prevent RMG from crashing, and we should avoid generating these types of structures in the first place. I think the filter on this PR is a sensible solution until we figure out how we want to generate resonance structs for adsorbates

mazeau

I think everything here looks fine. There definitely shouldn't be any bidentate things matching with vdw families.
I don't think the best thing to do is to trash species after we generate charges/radicals/lone pairs on surface sites, but to prevent rmg from suggesting them in the first place.

Yes, I agree. The fix on this PR is a band-aid to prevent RMG from crashing, and we should avoid generating these types of structures in the first place. I think the filter on this PR is a sensible solution until we figure out how we want to generate resonance structs for adsorbates

Yes, sorry, I forgot to change the bubble from comment to approve!

rmgpy/molecule/resonance.py

ChrisBNEU

This looks good, I agree with you and emily that this is a band-aid but hopefully when we have our surface resonance meetup with franklin we can revisit this. I tried it with an NO2/O2 on pt111 input file just to make sure it wouldn't give any errors. I had two concerns but other wise it looks ok

the commented out logging in resonance.py (see my other comment)
should we update the resonanceTest so we capture what we are doing for charged gas/surface and radical surface species?

davidfarinajr · 2021-05-24T15:52:02Z

should we update the resonanceTest so we capture what we are doing for charged gas/surface and radical surface species?

Yes, we should add unit test that makes resonance structs with radicals on X and make sure we filter them out (with this PR). We may have to change the test once we overhaul resonance generation for adsorbates, but that's ok. It could actually help to make sure future changes don't break this.

rmgpy/molecule/resonance.py

Tingchenlee

This looks good to me, thanks for the fixes!

rwest · 2021-05-26T16:46:50Z

There was a comment up thread that a unit test should be added. Are we waiting for that before merging, or has it been done and I missed it?

davidfarinajr · 2021-05-26T17:08:29Z

There was a comment up thread that a unit test should be added. Are we waiting for that before merging, or has it been done and I missed it?

I still need to add that unit test before merging. I also might get rid of the logging or change it to debug because it looks ugly in the RMG log files.

I'm also not completely satisfied with the way the filter works (makes bad structs then removes them). I can prevent them from being made, but I am not sure if that is a better long term solution since we may drastically change how resonance generation works for adsorbates 🤔

davidfarinajr · 2021-06-04T19:10:08Z

There was a comment up thread that a unit test should be added. Are we waiting for that before merging, or has it been done and I missed it?

I still need to add that unit test before merging. I also might get rid of the logging or change it to debug because it looks ugly in the RMG log files.

I'm also not completely satisfied with the way the filter works (makes bad structs then removes them). I can prevent them from being made, but I am not sure if that is a better long term solution since we may drastically change how resonance generation works for adsorbates 🤔

I added the unit test. I think the resonance fix on this PR is ok since it fixes a bug, but we should revisit resonance for adsorbates in the future.

davidfarinajr · 2021-06-04T19:11:17Z

rmgpy.exceptions.ChemkinError: 
Unexpected species identifier Estimated encountered in flux pairs for 
reaction H(6) + C2H4(10) <=> C2H5(5).

getting strange chemkin error in integration tests 🤔

rwest · 2021-06-04T19:56:48Z

I saw that. I think this commit just got merged via #2139 a couple hours ago - maybe even during the middle of running the tests. Let's rebase yet again and see if the problem goes away?

do not generate resonance for charges species, ensure input charge equals output charge, and discard adsorbates with radicals, lone pairs or charges on `X`

rwest

I requested you check one thing (atom label swaps), and chris had a comment (about logging stuff)) that's not been resolved yet. Otherwise looks good to me. So, very nearly ready to merge.

rmgpy/data/kinetics/family.py

One of the places where reactions that are their own reverse need to be hard-coded into RMG-Py

`Surface_Abstraction_Single_vdW` is generating products in the forward direction that violate the multiplcity of the template products

logging this as a warning clutters the RMG log files and it is not something the user should worry about

davidfarinajr · 2021-06-04T21:49:14Z

I requested you check one thing (atom label swaps), and chris had a comment (about logging stuff)) that's not been resolved yet. Otherwise looks good to me. So, very nearly ready to merge.

Thanks for the review Richard. Your label swaps for surface_abstraction_single_vdw are correct, so I removed it from my commit and cherrypicked yours onto this PR. The logging is not needed for resonance in my opinion, and is better left commented out.

Doing it this way, the string formatting is not evaluated if you aren't actually going to log the debug statements (usually the case, as we normally don't run in verbose mode). Should be marginally quicker.

rwest · 2021-06-05T04:00:34Z

This was meant to be catalysis fixes, and yet the regression tests are showing a change in a gas phase molecule's thermo:

Non-identical thermo!
original:	O1[C]=N1
tested:	O1[C]=N1
Hf(300K)  |S(300K)   |Cp(300K)  |Cp(400K)  |Cp(500K)  |Cp(600K)  |Cp(800K)  |Cp(1000K) |Cp(1500K) 
    116.46|     53.90|     11.62|     12.71|     13.49|     13.96|     14.14|     13.85|     13.58
    141.64|     58.66|     12.26|     12.27|     12.09|     11.96|     12.26|     12.72|     12.15
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(Cyclopropene) + radical(CdJ-NdO)
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(oxirene) + radical(CdJ-NdO)

is this a false alarm? Could we have somehow changed from ring(Cyclopropene) to ring(oxirene) by tweaking a resonance algorithm, or is this a prior non-deterministic bug in RMG and we got unlucky?

Looking at the code diff again, I can't think of anything that would cause this change. So maybe it's random?

...and so I check the issue tracker and sure enough: #2010

So, I'm satisfied it's not our fault, and am approving this PR.

davidfarinajr requested review from mazeau, Tingchenlee, ChrisBNEU and rwest May 21, 2021 00:29

mazeau reviewed May 21, 2021

View reviewed changes

mazeau previously approved these changes May 22, 2021

View reviewed changes

ChrisBNEU reviewed May 23, 2021

View reviewed changes

rmgpy/molecule/resonance.py Show resolved Hide resolved

ChrisBNEU reviewed May 24, 2021

View reviewed changes

Tingchenlee reviewed May 26, 2021

View reviewed changes

rmgpy/molecule/resonance.py Show resolved Hide resolved

Tingchenlee previously approved these changes May 26, 2021

View reviewed changes

davidfarinajr dismissed stale reviews from Tingchenlee and mazeau via 3c7a484 May 26, 2021 17:33

ChrisBNEU previously approved these changes May 26, 2021

View reviewed changes

davidfarinajr dismissed ChrisBNEU’s stale review via ea0f9a4 May 28, 2021 18:47

davidfarinajr force-pushed the cat_fixes branch 2 times, most recently from ea0f9a4 to 31b1feb Compare June 4, 2021 17:13

modifed resonance algorith for charged species

a2f5755

do not generate resonance for charges species, ensure input charge equals output charge, and discard adsorbates with radicals, lone pairs or charges on `X`

rwest force-pushed the cat_fixes branch from 31b1feb to 60ae246 Compare June 4, 2021 20:02

rwest reviewed Jun 4, 2021

View reviewed changes

rmgpy/data/kinetics/family.py Show resolved Hide resolved

davidfarinajr and others added 3 commits June 4, 2021 17:39

swap labels for own reverse surface_abstraction

7173182

Added label swaps for surface_abstraction_single_vdw

0cc4f95

One of the places where reactions that are their own reverse need to be hard-coded into RMG-Py

check the multiplicty of producst in the forward direction

4e98b40

`Surface_Abstraction_Single_vdW` is generating products in the forward direction that violate the multiplcity of the template products

davidfarinajr added 4 commits June 4, 2021 17:42

forbid vdw multi-dentate molecules for surface families

a363bf4

added test_molecule_forbidden unit test for vdw bidentates

2d90b1e

changed logging from warning to debug in resonance surface filter

e27d76e

logging this as a warning clutters the RMG log files and it is not something the user should worry about

added test_surface_radical resonance test

5374c6d

davidfarinajr force-pushed the cat_fixes branch from 60ae246 to 5374c6d Compare June 4, 2021 21:43

Using lazy string formatting in debug statements.

08c6343

Doing it this way, the string formatting is not evaluated if you aren't actually going to log the debug statements (usually the case, as we normally don't run in verbose mode). Should be marginally quicker.

rwest approved these changes Jun 5, 2021

View reviewed changes

rwest merged commit 7c468a4 into master Jun 5, 2021

rwest deleted the cat_fixes branch June 5, 2021 04:02

rwest mentioned this pull request Jun 5, 2021

Non-deterministic choice between ring(Cyclopropene) and ring(oxirene) ? #2010

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cat fixes - vdw radicals, bidentates, resonance #2136

Cat fixes - vdw radicals, bidentates, resonance #2136

davidfarinajr commented May 21, 2021 •

edited

codecov bot commented May 21, 2021 •

edited

mazeau left a comment

davidfarinajr commented May 21, 2021

mazeau left a comment

ChrisBNEU left a comment

davidfarinajr commented May 24, 2021

Tingchenlee left a comment

rwest commented May 26, 2021

davidfarinajr commented May 26, 2021

davidfarinajr commented Jun 4, 2021

davidfarinajr commented Jun 4, 2021

rwest commented Jun 4, 2021

rwest left a comment

davidfarinajr commented Jun 4, 2021

rwest commented Jun 5, 2021

Cat fixes - vdw radicals, bidentates, resonance #2136

Cat fixes - vdw radicals, bidentates, resonance #2136

Conversation

davidfarinajr commented May 21, 2021 • edited

Motivation or Problem and Description of Changes

Testing

codecov bot commented May 21, 2021 • edited

Codecov Report

mazeau left a comment

Choose a reason for hiding this comment

davidfarinajr commented May 21, 2021

mazeau left a comment

Choose a reason for hiding this comment

ChrisBNEU left a comment

Choose a reason for hiding this comment

davidfarinajr commented May 24, 2021

Tingchenlee left a comment

Choose a reason for hiding this comment

rwest commented May 26, 2021

davidfarinajr commented May 26, 2021

davidfarinajr commented Jun 4, 2021

davidfarinajr commented Jun 4, 2021

rwest commented Jun 4, 2021

rwest left a comment

Choose a reason for hiding this comment

davidfarinajr commented Jun 4, 2021

rwest commented Jun 5, 2021

davidfarinajr commented May 21, 2021 •

edited

codecov bot commented May 21, 2021 •

edited