FIX: Updated logical expression to remove short circuit #2555

fazledyn-or · 2023-10-09T12:40:37Z

Motivation or Problem

While triaging your project, our bug fixing tool generated the following message(s)-

In file: solvation.py, method: _add_ring_correction_solute_data_from_tree, a logical expression uses the identity operator. A new object is created inside the identity check operation and then used for matching identity. Since this is a distinct, new object, it will not have identity an match with anything else. As a result, the identity check will have a logical short circuit and the program may have unintended behavior. iCR suggested that the logical operation should be reviewed for correctness.

Code Snippet

      if matched_ring_entries is []:
          raise KeyError('Node not found in database.')
      # Decide which group to keep

A simple test like below demonstrates the logical bug -

Description of Changes

Changed the is operator with == to fix the logical bug.

Testing

No local tests have been performed yet. Facing problems in installation and testing.

Reviewer Tips

Please review and suggest as seems fine.

CLA Requirements

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

All contributed commits are already automatically signed off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).
- Git Commit SignOff documentation

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

fazledyn-or · 2023-10-09T12:46:29Z

A similar logical bug was found previously in this project: #2542

JacksonBurns

LGTM

codecov · 2023-10-09T23:16:20Z

Codecov Report

Merging #2555 (d710502) into main (76ecd99) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2555   +/-   ##
=======================================
  Coverage   55.32%   55.32%           
=======================================
  Files         125      125           
  Lines       37143    37143           
=======================================
  Hits        20548    20548           
  Misses      16595    16595

Files	Coverage Δ
rmgpy/data/solvation.py	`59.26% <100.00%> (ø)`

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

github-actions · 2023-10-09T23:17:38Z

Regression Testing Results

⚠️ 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:25
Current: Execution time (DD:HH:MM:SS): 00:00:01:24
Reference: Memory used: 2137.96 MB
Current: Memory used: 2107.23 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 Failed Edge Comparison ❌

Original model has 106 species.
Test model has 106 species. ✅
Original model has 358 reactions.
Test model has 358 reactions. ✅

Non-identical thermo! ❌
original: C=CC1C=CC2=CC1C=C2
tested: C=CC1C=CC2=CC1C=C2

Hf(300K)	S(300K)	Cp(300K)	Cp(400K)	Cp(500K)	Cp(600K)	Cp(800K)	Cp(1000K)	Cp(1500K)
83.22	82.78	35.48	45.14	53.78	61.40	73.58	82.20	95.08
83.22	84.16	35.48	45.14	53.78	61.40	73.58	82.20	95.08

Identical thermo comments:
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)(Cds-Cds)CsH) + group(Cs-(Cds-Cds)(Cds-Cds)CsH) + group(Cds-Cds(Cds-Cds)(Cds-Cds)) + group(Cds- CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)H) + group(Cds-CdsHH) + Estimated bicyclic component: polycyclic(s3_5_6_ane) - ring(Cyclohexane) - ring(Cyclopentane) + ring(1,3-Cyclohexadiene) + ring(Cyclopentadiene)

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:53
Current: Execution time (DD:HH:MM:SS): 00:00:02:49
Reference: Memory used: 2225.27 MB
Current: Memory used: 2225.99 MB

liquid_oxidation Failed Core Comparison ❌

Original model has 37 species.
Test model has 37 species. ✅
Original model has 215 reactions.
Test model has 216 reactions. ❌
The tested model has 1 reactions that the original model does not have. ❌
rxn: CCO[O](29) <=> [OH](22) + CC=O(69) origin: intra_H_migration

Non-identical kinetics! ❌
original:
rxn: CCCC(C)O[O](20) + CCCCCO[O](104) <=> oxygen(1) + CCCC(C)[O](61) + CCCCC[O](128) origin: Peroxyl_Disproportionation
tested:
rxn: CCCC(C)O[O](20) + CCCCCO[O](104) <=> oxygen(1) + CCCC(C)[O](61) + CCCCC[O](127) 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.77	4.45	4.86	5.14	5.48	5.68	5.96	6.09

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.756,'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

liquid_oxidation Failed Edge Comparison ❌

Original model has 202 species.
Test model has 202 species. ✅
Original model has 1613 reactions.
Test model has 1610 reactions. ❌
The original model has 5 reactions that the tested model does not have. ❌
rxn: CCO[O](30) <=> C[CH]OO(70) origin: intra_H_migration
rxn: C[CH]CCCO(157) + CCCCCO[O](104) <=> CC=CCCO(192) + CCCCCOO(105) origin: Disproportionation
rxn: C[CH]CCCO(157) + CCCCCO[O](104) <=> C=CCCCO(193) + CCCCCOO(105) origin: Disproportionation
rxn: C[CH]CCCO(157) + C[CH]CCCO(157) <=> CC=CCCO(192) + CCCCCO(130) origin: Disproportionation
rxn: C[CH]CCCO(157) + C[CH]CCCO(157) <=> C=CCCCO(193) + CCCCCO(130) origin: Disproportionation
The tested model has 2 reactions that the original model does not have. ❌
rxn: CCO[O](29) <=> [OH](22) + CC=O(69) origin: intra_H_migration
rxn: CCCCCO[O](104) + CCCCCO[O](104) <=> oxygen(1) + CCCCC=O(106) + CCCCCO(130) origin: Peroxyl_Termination

Non-identical kinetics! ❌
original:
rxn: CCCC(C)O[O](20) + CCCCCO[O](104) <=> oxygen(1) + CCCC(C)[O](61) + CCCCC[O](128) origin: Peroxyl_Disproportionation
tested:
rxn: CCCC(C)O[O](20) + CCCCCO[O](104) <=> oxygen(1) + CCCC(C)[O](61) + CCCCC[O](127) 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.77	4.45	4.86	5.14	5.48	5.68	5.96	6.09

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.756,'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

Non-identical kinetics! ❌
original:
rxn: CCCCCO[O](104) + CC(CC(C)OO)O[O](103) <=> oxygen(1) + CCCCC[O](128) + CC([O])CC(C)OO(127) origin: Peroxyl_Disproportionation
tested:
rxn: CCCCCO[O](104) + CC(CC(C)OO)O[O](103) <=> oxygen(1) + CCCCC[O](127) + CC([O])CC(C)OO(129) origin: Peroxyl_Disproportionation

k(1bar)	300K	400K	500K	600K	800K	1000K	1500K	2000K
k(T):	3.52	4.27	4.71	5.01	5.39	5.61	5.91	6.06
k(T):	7.79	7.46	7.21	7.00	6.67	6.41	5.94	5.60

kinetics: Arrhenius(A=(3.2e+12,'cm^3/(mol*s)'), n=0, Ea=(4.096,'kcal/mol'), T0=(1,'K'), comment="""Estimated from node Root_Ext-5R-R_7R!H->C_N-7C-inRing_Ext-5R-R""")
kinetics: Arrhenius(A=(3.18266e+20,'cm^3/(mol*s)'), n=-2.694, Ea=(0.053,'kcal/mol'), T0=(1,'K'), comment="""Estimated from node Root_Ext-5R-R_7R!H->C_N-7C-inRing Ea raised from 0.0 to 0.2 kJ/mol to match endothermicity of reaction.""")
kinetics: 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
Ea raised from 0.0 to 0.2 kJ/mol to match endothermicity of reaction.

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:48
Current: Execution time (DD:HH:MM:SS): 00:00:01:47
Reference: Memory used: 2223.43 MB
Current: Memory used: 2223.19 MB

nitrogen Passed Core Comparison ✅

Original model has 41 species.
Test model has 41 species. ✅
Original model has 360 reactions.
Test model has 360 reactions. ✅

nitrogen Passed Edge Comparison ✅

Original model has 132 species.
Test model has 132 species. ✅
Original model has 997 reactions.
Test model has 997 reactions. ✅

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:03:25
Current: Execution time (DD:HH:MM:SS): 00:00:03:26
Reference: Memory used: 2079.06 MB
Current: Memory used: 2083.37 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:01:10
Current: Execution time (DD:HH:MM:SS): 00:00:01:09
Reference: Memory used: 2181.71 MB
Current: Memory used: 2192.77 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:44
Current: Execution time (DD:HH:MM:SS): 00:00:00:43
Reference: Memory used: 2344.55 MB
Current: Memory used: 2351.78 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:03:21
Current: Execution time (DD:HH:MM:SS): 00:00:03:15
Reference: Memory used: 2753.90 MB
Current: Memory used: 2757.22 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:08:25
Current: Execution time (DD:HH:MM:SS): 00:00:08:06
Reference: Memory used: 2733.58 MB
Current: Memory used: 2724.13 MB

RMS_CSTR_liquid_oxidation Passed Core Comparison ✅

Original model has 37 species.
Test model has 37 species. ✅
Original model has 233 reactions.
Test model has 233 reactions. ✅

RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌

Original model has 205 species.
Test model has 206 species. ❌
Original model has 1513 reactions.
Test model has 1508 reactions. ❌
The tested model has 1 species that the original model does not have. ❌
spc: [O]OOO(19)
The original model has 6 reactions that the tested model does not have. ❌
rxn: [CH2]CC(5) + [CH2]CCCC(12) <=> C=CC(26) + pentane(2) origin: Disproportionation
rxn: [OH](21) + CCCOO(63) <=> O(40) + C[CH]COO(45) origin: H_Abstraction
rxn: [OH](21) + CCCOO(63) <=> O(40) + CC[CH]OO(54) origin: H_Abstraction
rxn: [OH](21) + CCCOO(63) <=> O(40) + [CH2]CCOO(46) origin: H_Abstraction
rxn: CCOO(65) + CCCOO(63) <=> O(40) + CC[O](100) + CCCO[O](35) origin: Bimolec_Hydroperoxide_Decomposition
rxn: CCOO(65) + CCCOO(63) <=> O(40) + CCO[O](36) + CCC[O](76) origin: Bimolec_Hydroperoxide_Decomposition
The tested model has 1 reactions that the original model does not have. ❌
rxn: oxygen(1) + [O]O(13) <=> [O]OOO(19) origin: R_Recombination

Non-identical kinetics! ❌
original:
rxn: CCCO[O](35) + CCCC(C)O[O](33) <=> oxygen(1) + CCC[O](76) + CCCC(C)[O](66) origin: Peroxyl_Disproportionation
tested:
rxn: CCCO[O](35) + CCCC(C)O[O](33) <=> oxygen(1) + CCC[O](98) + CCCC(C)[O](65) 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 ✅

beep boop this comment was written by a bot 🤖

FIX: Updated logical expression to remove short circuit

d710502

JacksonBurns self-requested a review October 9, 2023 20:52

JacksonBurns approved these changes Oct 9, 2023

View reviewed changes

JacksonBurns enabled auto-merge October 9, 2023 20:52

JacksonBurns merged commit afdb597 into ReactionMechanismGenerator:main Oct 9, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Updated logical expression to remove short circuit #2555

FIX: Updated logical expression to remove short circuit #2555

fazledyn-or commented Oct 9, 2023 •

edited

Loading

fazledyn-or commented Oct 9, 2023

JacksonBurns left a comment

codecov bot commented Oct 9, 2023

github-actions bot commented Oct 9, 2023

Regression test aromatics:

Regression test liquid_oxidation:

Regression test nitrogen:

Regression test oxidation:

Regression test sulfur:

Regression test superminimal:

Regression test RMS_constantVIdealGasReactor_superminimal:

Regression test RMS_CSTR_liquid_oxidation:

FIX: Updated logical expression to remove short circuit #2555

FIX: Updated logical expression to remove short circuit #2555

Conversation

fazledyn-or commented Oct 9, 2023 • edited Loading

Motivation or Problem

Code Snippet

Description of Changes

Testing

Reviewer Tips

CLA Requirements

Sponsorship and Support

fazledyn-or commented Oct 9, 2023

JacksonBurns left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 9, 2023

Codecov Report

github-actions bot commented Oct 9, 2023

Regression Testing Results

Regression test aromatics:

Regression test liquid_oxidation:

Regression test nitrogen:

Regression test oxidation:

Regression test sulfur:

Regression test superminimal:

Regression test RMS_constantVIdealGasReactor_superminimal:

Regression test RMS_CSTR_liquid_oxidation:

fazledyn-or commented Oct 9, 2023 •

edited

Loading