search

ReactionMechanismGenerator / RMG-Py

Python version of the amazing Reaction Mechanism Generator (RMG).
http://reactionmechanismgenerator.github.io/RMG-Py/
Other
400 stars 228 forks source link

Draw Hydrogen bonds (again) #2734

Closed rwest closed 6 days ago

rwest commented 1 week ago

Motivation or Problem

Pull request https://github.com/ReactionMechanismGenerator/RMG-Py/pull/1961 introduced the ability to draw hydrogen bonds.

To do so, Hydrogen atoms that have H bonds shouldn't be removed from the drawings. This check was removed without explanation in ec9515351ea9964a5a8499c1f4066378544eca41 which was designed to make drawing work for fragments. That broke drawing of hydrogen bonds. The unit test for drawing hydrogen bonds exists, but just checks that a PDF is made, not what it looks like. So the regression was not detected.

Description of Changes

This restores the check that H atoms in H-bonds are not removed from drawings. It also removes code duplication that was introduced in #1961 through erroneous rebasing, that @bjkreitz noticed and removed in a different PR.

Testing

It was while testing the second commit (Bjarne's commit that removes the code duplication) that I found the title problem (the H bonds being broken). Now I've tested both, by running the original tests from #1961 

example =  Molecule().from_adjacency_list("""
example
1  O u0 p3 c-1 {2,S} {10,H}
2  N u0 p0 c+1 {1,S} {3,D} {4,S}
3  O u0 p2 c0 {2,D}
4  O u0 p2 c0 {2,S} {7,S}
5  N u0 p1 c0 {6,S} {8,S} {9,S} {7,H}
6  O u0 p2 c0 {5,S} {10,S}
7  H u0 p0 c0 {4,S} {5,H}
8  H u0 p0 c0 {5,S}
9  H u0 p0 c0 {5,S}
10 H u0 p0 c0 {6,S} {1,H}
""")
example
display(example)
for m in example.generate_h_bonded_structures():
    display(m)

Restoring the check makes the H bonds work. This particular molecule still looks ugly, as it always did. Removing the duplicate code doesn't make them worse.

Reviewer Tips

Not a big change, and not worth a lot of time.

github-actions[bot] commented 1 week ago

Regression Testing Results

WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.

Detailed regression test results. ### Regression test aromatics: Reference: Execution time (DD:HH:MM:SS): 00:00:01:05 Current: Execution time (DD:HH:MM:SS): 00:00:01:04 Reference: Memory used: 2759.25 MB Current: Memory used: 2789.68 MB
aromatics Passed Core Comparison ✅ Original model has 15 species. Test model has 15 species. ✅ Original model has 11 reactions. Test model has 11 reactions. ✅
aromatics Passed Edge Comparison ✅ Original model has 106 species. Test model has 106 species. ✅ Original model has 358 reactions. Test model has 358 reactions. ✅
Observables Test Case: Aromatics Comparison ✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅
### Regression test liquid_oxidation: Reference: Execution time (DD:HH:MM:SS): 00:00:02:08 Current: Execution time (DD:HH:MM:SS): 00:00:02:06 Reference: Memory used: 2890.78 MB Current: Memory used: 2898.54 MB
liquid_oxidation Passed Core Comparison ✅ Original model has 37 species. Test model has 37 species. ✅ Original model has 216 reactions. Test model has 216 reactions. ✅
liquid_oxidation Passed Edge Comparison ✅ Original model has 202 species. Test model has 202 species. ✅ Original model has 1610 reactions. Test model has 1610 reactions. ✅
Observables Test Case: liquid_oxidation Comparison ✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅
### Regression test nitrogen: Reference: Execution time (DD:HH:MM:SS): 00:00:01:25 Current: Execution time (DD:HH:MM:SS): 00:00:01:24 Reference: Memory used: 2897.56 MB Current: Memory used: 2900.09 MB
nitrogen Failed Core Comparison ❌ Original model has 41 species. Test model has 41 species. ✅ Original model has 360 reactions. Test model has 359 reactions. ❌ The original model has 1 reactions that the tested model does not have. ❌ rxn: `HNO(48) + HCO(13) <=> NO(38) + CH2O(18)` origin: H_Abstraction
nitrogen Failed Edge Comparison ❌ Original model has 133 species. Test model has 133 species. ✅ Original model has 983 reactions. Test model has 981 reactions. ❌ 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) | |----------|----------|----------|----------|----------|----------|----------|----------|----------| | 141.64| 58.66| 12.26| 12.27| 12.09| 11.96| 12.26| 12.72| 12.15| | 116.46| 53.90| 11.62| 12.71| 13.49| 13.96| 14.14| 13.85| 13.58| thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(oxirene) + radical(CdJ-NdO) thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(Cyclopropene) + radical(CdJ-NdO) The original model has 2 reactions that the tested model does not have. ❌ rxn: `HNO(48) + HCO(13) <=> NO(38) + CH2O(18)` origin: H_Abstraction rxn: `HON(T)(83) + HCO(13) <=> NO(38) + CH2O(18)` origin: Disproportionation Non-identical kinetics! ❌ original: rxn: `NCO(66) <=> O1[C]=N1(126)` origin: Intra_R_Add_Endocyclic tested: rxn: `NCO(66) <=> O1[C]=N1(126)` origin: Intra_R_Add_Endocyclic |k(1bar)|300K |400K |500K |600K |800K |1000K |1500K |2000K | |-------|-------|-------|-------|-------|-------|-------|-------|-------| |k(T): | -66.25| -46.19| -34.19| -26.21| -16.28| -10.36| -2.54| 1.31| |k(T): | -49.54| -33.65| -24.16| -17.85| -10.01| -5.35| 0.80| 3.82| kinetics: `Arrhenius(A=(6.95187e+18,'s^-1'), n=-1.628, Ea=(111.271,'kcal/mol'), T0=(1,'K'), comment="""Estimated from node Backbone0_N-2R!H-inRing_N-1R!H-inRing_Sp-2R!H-1R!H""")` kinetics: `Arrhenius(A=(6.95187e+18,'s^-1'), n=-1.628, Ea=(88.327,'kcal/mol'), T0=(1,'K'), comment="""Estimated from node Backbone0_N-2R!H-inRing_N-1R!H-inRing_Sp-2R!H-1R!H""")` Identical kinetics comments: kinetics: Estimated from node Backbone0_N-2R!H-inRing_N-1R!H-inRing_Sp-2R!H-1R!H
Observables Test Case: NC Comparison ✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅
### Regression test oxidation: Reference: Execution time (DD:HH:MM:SS): 00:00:02:26 Current: Execution time (DD:HH:MM:SS): 00:00:02:19 Reference: Memory used: 2748.37 MB Current: Memory used: 2765.68 MB
oxidation Passed Core Comparison ✅ Original model has 59 species. Test model has 59 species. ✅ Original model has 694 reactions. Test model has 694 reactions. ✅
oxidation Passed Edge Comparison ✅ Original model has 230 species. Test model has 230 species. ✅ Original model has 1526 reactions. Test model has 1526 reactions. ✅
Observables Test Case: Oxidation Comparison ✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅
### Regression test sulfur: Reference: Execution time (DD:HH:MM:SS): 00:00:00:53 Current: Execution time (DD:HH:MM:SS): 00:00:00:52 Reference: Memory used: 2847.88 MB Current: Memory used: 2878.36 MB
sulfur Passed Core Comparison ✅ Original model has 27 species. Test model has 27 species. ✅ Original model has 74 reactions. Test model has 74 reactions. ✅
sulfur Failed Edge Comparison ❌ Original model has 89 species. Test model has 89 species. ✅ Original model has 227 reactions. Test model has 227 reactions. ✅ The original model has 1 reactions that the tested model does not have. ❌ rxn: `O(4) + SO2(15) (+N2) <=> SO3(16) (+N2)` origin: primarySulfurLibrary The tested model has 1 reactions that the original model does not have. ❌ rxn: `O(4) + SO2(15) (+N2) <=> SO3(16) (+N2)` origin: primarySulfurLibrary
Observables Test Case: SO2 Comparison ✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! sulfur Passed Observable Testing ✅
### Regression test superminimal: Reference: Execution time (DD:HH:MM:SS): 00:00:00:35 Current: Execution time (DD:HH:MM:SS): 00:00:00:34 Reference: Memory used: 2965.46 MB Current: Memory used: 2956.12 MB
superminimal Passed Core Comparison ✅ Original model has 13 species. Test model has 13 species. ✅ Original model has 21 reactions. Test model has 21 reactions. ✅
superminimal Passed Edge Comparison ✅ Original model has 18 species. Test model has 18 species. ✅ Original model has 28 reactions. Test model has 28 reactions. ✅
### Regression test RMS_constantVIdealGasReactor_superminimal: Reference: Execution time (DD:HH:MM:SS): 00:00:02:24 Current: Execution time (DD:HH:MM:SS): 00:00:02:19 Reference: Memory used: 3522.57 MB Current: Memory used: 3486.84 MB
RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅ Original model has 13 species. Test model has 13 species. ✅ Original model has 19 reactions. Test model has 19 reactions. ✅
RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅ Original model has 13 species. Test model has 13 species. ✅ Original model has 19 reactions. Test model has 19 reactions. ✅
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison ✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅
### Regression test RMS_CSTR_liquid_oxidation: Reference: Execution time (DD:HH:MM:SS): 00:00:05:55 Current: Execution time (DD:HH:MM:SS): 00:00:05:44 Reference: Memory used: 3414.52 MB Current: Memory used: 3411.63 MB
RMS_CSTR_liquid_oxidation Failed Core Comparison ❌ Original model has 37 species. Test model has 37 species. ✅ Original model has 233 reactions. Test model has 232 reactions. ❌ The original model has 1 reactions that the tested model does not have. ❌ rxn: `CCO[O](34) <=> [OH](21) + CC=O(62)` origin: intra_H_migration
RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌ Original model has 206 species. Test model has 206 species. ✅ Original model has 1508 reactions. Test model has 1508 reactions. ✅ The original model has 2 reactions that the tested model does not have. ❌ rxn: `CCO[O](34) <=> [OH](21) + CC=O(62)` origin: intra_H_migration rxn: `CCCO[O](35) <=> [OH](21) + CCC=O(44)` origin: intra_H_migration The tested model has 2 reactions that the original model does not have. ❌ rxn: `CCCO[O](34) <=> CC[CH]OO(45)` origin: intra_H_migration rxn: `CCO[O](36) <=> C[CH]OO(62)` origin: intra_H_migration Non-identical kinetics! ❌ original: rxn: `CCCO[O](35) + CCCC(C)O[O](33) <=> oxygen(1) + CCC[O](92) + CCCC(C)[O](65)` origin: Peroxyl_Disproportionation tested: rxn: `CCCO[O](34) + CCCC(C)O[O](33) <=> oxygen(1) + CCC[O](94) + CCCC(C)[O](61)` origin: Peroxyl_Disproportionation |k(1bar)|300K |400K |500K |600K |800K |1000K |1500K |2000K | |-------|-------|-------|-------|-------|-------|-------|-------|-------| |k(T): | 7.83| 7.49| 7.23| 7.02| 6.68| 6.42| 5.95| 5.61| |k(T): | 3.69| 4.39| 4.82| 5.10| 5.45| 5.66| 5.94| 6.08| kinetics: `Arrhenius(A=(3.18266e+20,'cm^3/(mol*s)'), n=-2.694, Ea=(0,'kcal/mol'), T0=(1,'K'), comment="""Estimated from node Root_Ext-5R-R_7R!H->C_N-7C-inRing""")` kinetics: `Arrhenius(A=(3.2e+12,'cm^3/(mol*s)'), n=0, Ea=(3.866,'kcal/mol'), T0=(1,'K'), comment="""Estimated from node Root_Ext-5R-R_7R!H->C_N-7C-inRing_Ext-5R-R""")` kinetics: Estimated from node Root_Ext-5R-R_7R!H->C_N-7C-inRing kinetics: Estimated from node Root_Ext-5R-R_7R!H->C_N-7C-inRing_Ext-5R-R
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison ✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅
### Regression test fragment: Reference: Execution time (DD:HH:MM:SS): 00:00:00:40 Current: Execution time (DD:HH:MM:SS): 00:00:00:39 Reference: Memory used: 2680.55 MB Current: Memory used: 2697.44 MB
fragment Passed Core Comparison ✅ Original model has 10 species. Test model has 10 species. ✅ Original model has 2 reactions. Test model has 2 reactions. ✅
fragment Passed Edge Comparison ✅ Original model has 33 species. Test model has 33 species. ✅ Original model has 47 reactions. Test model has 47 reactions. ✅
Observables Test Case: fragment Comparison ✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! fragment Passed Observable Testing ✅
### Regression test RMS_constantVIdealGasReactor_fragment: Reference: Execution time (DD:HH:MM:SS): 00:00:03:04 Current: Execution time (DD:HH:MM:SS): 00:00:02:58 Reference: Memory used: 3652.67 MB Current: Memory used: 3630.89 MB
RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅ Original model has 10 species. Test model has 10 species. ✅ Original model has 2 reactions. Test model has 2 reactions. ✅
RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅ Original model has 27 species. Test model has 27 species. ✅ Original model has 24 reactions. Test model has 24 reactions. ✅
Observables Test Case: RMS_constantVIdealGasReactor_fragment Comparison ✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_fragment Passed Observable Testing ✅
### Regression test minimal_surface: Reference: Execution time (DD:HH:MM:SS): 00:00:00:43 Current: Execution time (DD:HH:MM:SS): 00:00:00:42 Reference: Memory used: 2852.16 MB Current: Memory used: 2877.33 MB
minimal_surface Passed Core Comparison ✅ Original model has 11 species. Test model has 11 species. ✅ Original model has 3 reactions. Test model has 3 reactions. ✅
minimal_surface Passed Edge Comparison ✅ Original model has 38 species. Test model has 38 species. ✅ Original model has 38 reactions. Test model has 38 reactions. ✅
Observables Test Case: minimal_surface Comparison ✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! minimal_surface Passed Observable Testing ✅

beep boop this comment was written by a bot :robot: