Modifications to Arrhenius BM's get_w0() to enable halocarbene_recombination_double retraining - #2950
Modifications to Arrhenius BM's get_w0() to enable halocarbene_recombination_double retraining#2950Nora-Khalil wants to merge 2 commits into
Conversation
| previous_action = recipe[ind-1] | ||
|
|
||
| #make sure this error occurred because there was FORM_BOND and then CHANGE_BOND performed between the same two atoms. | ||
| assert previous_action[0]=='FORM_BOND' and current_action[1:]==previous_action[1:], f'{e} This is NOT because CHANGE_BOND was performed on a bond that was formed in the action step before.' |
There was a problem hiding this comment.
Hey, sorry to be super pedantic, but you should run a linter to highlight the PEP8 violations (mostly rules for spacing, I recommend the Flake8 extension in VSCode). I know, the rest of the file isn't compliant either, but the new stuff adds a bunch of angry red squigglies in my code editor.
The two main rule violations I'm seeing are that 1. your comments are supposed to have at least one space after the '#' and 2. operators like == should have spaces on either side.
| bd1 = mol.get_bond(atom1, atom2) | ||
| except ValueError as e: | ||
| #confirm that this is the error we need to catch | ||
| if 'The specified vertices are not connected by an edge in this graph.' in str(e): |
There was a problem hiding this comment.
I think you should add a corresponding "else"
if there's a ValueError and it's not the specific case this is designed to catch, you should re-raise the value error. I think the command is just raise e
|
I tested this fix on I will note that I was also able to get Anyways, this looks good to me and I'll approve it once the above suggestions are in. |
|
Hi @sevyharris just added in a statement that raises e if the ValueError is not one we expected. Thanks for the suggestion! And I'll look into the other families that also get the ValueError...but later :) Thanks! |
|
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. |
…ses when trying to calculate the w0 for a family that has consecutive reaction recipe steps of FORM_BOND and CHANGE_BOND between the same two atoms (i.e. reaction recipes where there is no bond formation between two atoms in the reactants, and then a double bond formation between the two atoms in the product). I believe this is only affecting halocarbene_recombination_double kinetic family.
909e2b1 to
b5c0956
Compare
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:55 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)(Cds-Cds)CsH) + group(Cs-(Cds-Cds)CsHH) + group(Cds-CdsCsCs) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + polycyclic(s2_4_5_diene_1_5) + polycyclic(s3_4_5_ene_3) + polycyclic(s2_5_5_diene_1_5) - ring(Cyclobutene) - ring(Cyclopentene) - ring(Cyclopentene) + radical(cyclopentene-allyl) Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)(Cds-Cds)CsCs) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-CsCsHH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + polycyclic(s2_4_4_ene_1) + polycyclic(s1_4_5_diene_1_6) + polycyclic(s3_4_5_ene_1) - ring(Cyclobutene) - ring(Cyclobutane) - ring(Cyclopentene) + radical(bicyclo[2.1.1]hex-2-ene-C5) Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-CsCsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsHH) + group(Cds- Cds(Cds-Cds)Cs) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + polycyclic(s2_3_5_ene_1) + polycyclic(s2_3_6_ene_1) + Estimated bicyclic component: polycyclic(s3_5_6_ane) - ring(Cyclohexane) - ring(Cyclopentane) + ring(Cyclohexene) + ring(Cyclopentene) - ring(Cyclopropane) - ring(Cyclopentene) - ring(Cyclohexene) + radical(cyclopentene-allyl) Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-CsCsHH) + group(Cds- Cds(Cds-Cds)Cs) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + polycyclic(s2_4_4_ene_1) + polycyclic(s3_4_6_ene_1) + Estimated bicyclic component: polycyclic(s2_4_6_ane) - ring(Cyclohexane) - ring(Cyclobutane) + ring(Cyclohexene) + ring(Cyclobutene) - ring(Cyclobutane) - ring(Cyclobutene) - ring(Cyclohexene) + radical(cyclobutane) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Errors occurred during edge comparison
ERROR conda.cli.main_run:execute(148): `conda run python scripts/checkModels.py aromatics-edge stable_regression_results/aromatics/chemkin/chem_edge_annotated.inp stable_regression_results/aromatics/chemkin/species_edge_dictionary.txt test/regression/aromatics/chemkin/chem_edge_annotated.inp test/regression/aromatics/chemkin/species_edge_dictionary.txt` failed. (See above for error)
|
2 participants
Motivation or Problem
The halocarbene_recombination_double family recipe is written as follows:
where there is no bond between atoms *1 and *2 in the reactants, and a double bond between *1 and *2 in the product.
This causes an error when using these reactions to fit a ArrheniusBM. When
get_w0()is called for calculating the w0 for the BM's kinetics, a ValueError is raised:ValueError: The specified vertices are not connected by an edge in this graphThis is because the current code in get_w0 will attempt to calculate the total bond energy of bonds formed (wf) by first finding the difference between the bond dissociation energy of bond between *1 and *2 in the reactants and the bond between *1 and *2 in the product. The code doesn't seem to expect that there would be no bond between *1 and *2 in the reactants if the reaction recipe has a CHANGE_BOND step.
I iterated through all families in 'default' looking for recipes that form a bond between two atoms and then change the bond (i.e. going from no bond in reactants to double or higher bond in product). I could only find halocarbene_recombination_double that has this, so I believe this PR fix is only needed for the retraining of this family.
Description of Changes
Implemented a try/except that catches ValueError, confirms that it is an example of the problem case described above, then creates a Bond object with an order of 0 (so code downstream won't crash) and a bond dissociation energy of 0.0 so the calculation for wf is still accurate.
Was able to successfully retrain family with this fix.
Testing
Here's some simple code for testing...
Before PR, ValueError would be caught in following code snippet. After PR, should pass:
Thanks!