Update extract_source_from_comments to grab library object in addition to library name#2963
Conversation
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:52 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(s3_5_5_ene_1) - ring(Cyclobutene) - ring(Cyclopentene) - ring(Cyclopentane) + radical(cyclopentene-allyl) Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)(Cds-Cds)CsH) + group(Cs-(Cds-Cds)(Cds-Cds)CsH) + group(Cs-CsCsHH) + group(Cds-CdsCsCs) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + Estimated bicyclic component: polycyclic(s2_3_5_ane) - ring(Cyclopentane) - ring(Cyclopropane) + ring(Cyclopentene) + ring(Cyclopropane) + polycyclic(s2_3_6_diene_0_3) + Estimated bicyclic component: polycyclic(s3_5_6_ane) - ring(Cyclohexane) - ring(Cyclopentane) + ring(1,4-Cyclohexadiene) + ring(Cyclopentene) - ring(Cyclopropane) - ring(Cyclopentene) - ring(1,4-Cyclohexadiene) + radical(cyclopentene-4) 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_diene_1_3) + Estimated bicyclic component: polycyclic(s3_5_6_ane) - ring(Cyclohexane) - ring(Cyclopentane) + ring(1,3-Cyclohexadiene) + ring(Cyclopentene) - ring(Cyclopropane) - ring(Cyclopentene) - ring(1,3-Cyclohexadiene) + radical(cyclopentene-allyl) Non-identical thermo! ❌
Identical thermo comments: 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)
|
sevyharris
force-pushed
the
expand_lib_extract_source
branch
7 times, most recently
from
daa9af4 to
a167215
Compare
sevyharris
force-pushed
the
expand_lib_extract_source
branch
from
a167215 to
b9eeaf0
Compare
sevyharris
force-pushed
the
expand_lib_extract_source
branch
from
b9eeaf0 to
1e43cf9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
rmgpy/tools/uncertainty.py:78
get_uncertainty_value()constructs the covariance lookup label asf'Surface_Library {source["Surface_Library"][2]}', butsource['Surface_Library'][2]is already intended to be the full parameter label (e.g.,Surface_Library CH4(16)), so this will duplicate the prefix and fail to match keys inother_covariances. Use the stored label directly (and ensureextract_sources_from_model()stores it in the same format used when building covariance labels).
varG += self.dG_library * self.dG_library
if 'Surface_Library' in source:
surf_lib_varG = self.dG_surf_lib * self.dG_surf_lib
# covariance libraries should overrule the default uncertainties when available,
if self.other_covariances is not None:
label = f'Surface_Library {source["Surface_Library"][2]}' # match the covariance dict label format
cov_label = (label, label)
if cov_label in self.other_covariances:
surf_lib_varG = self.other_covariances[cov_label]
varG += surf_lib_varG # Add the variance of the surface library parameter if covariance is not specified in the covariance libraries
a0dfbfd to
6b61784
Compare
Add some tests to make sure the correct index is referenced when assembling thermo sources that use libraries. For example, if CH3 is estimated from a CH4 library + a radical correction, we want to make sure the uncertainty source points to CH4 as the index for the library value used (as opposed to CH3)
sevyharris
force-pushed
the
expand_lib_extract_source
branch
from
6b61784 to
e384ea8
Compare
This refactors the extract_source_from_comments function and changes library sources so that a tuple of the library name and the library entry object is returned instead of just the library name. It also returns a tuple of QM method and molecule used for the case of QM sources.
sevyharris
force-pushed
the
expand_lib_extract_source
branch
from
e384ea8 to
41ada7e
Compare
Before this, the kinetics version of extract_source_from_comment returned the library name, but now it returns a tuple of the library name and the library entry. This makes things much easier downstream in the uncertainty tool.
sevyharris
force-pushed
the
expand_lib_extract_source
branch
from
41ada7e to
438222f
Compare
When the library sources only included the name of the library, the uncertainty code had to compensate by adding extra species that are not in the model. Now that we're using library entry objects we are able to remove the complexity of the uncertainty.extra_species object.
sevyharris
force-pushed
the
expand_lib_extract_source
branch
from
438222f to
aedb962
Compare
Dependencies
Motivation or Problem: we need this to implement grabbing rank for UQ (not part of RMG 4 paper)
Right now
extract_source_from_commentsRMG-Py/rmgpy/data/thermo.py
Line 2838 in 8a0b7fe
only grabs the library name instead of the library entry object. This causes problems downstream in the uncertainty estimation tool because
get_partial_uncertainty_valueRMG-Py/rmgpy/tools/uncertainty.py
Line 91 in 8a0b7fe
only has access to what gets saved in the source, and it's a total pain extracting things like the species structure or rank, which could have been added in
extract_source_from_commentsand are added for Rate Rules, but never got implemented for Libraries.This PR updates
extract_source_from_commentsto use the actual library entry objects, and then modifies the downstream code in rmgpy.tools.uncertainty to use that information.Description of Changes (each bullet is a commit)
_parse_gav_groupsis almost an exact copy of the last section of the oldextract_source_from_comments) and also return specific library entry objects instead of just the library name. Update thermoTest to handle this change in library source format.Testing
Reviewer Tips
¯_(ツ)_/¯