More helpful debugging for PDep networks, and option to always generatePESDiagrams. #2825
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances debugging for pressure-dependent networks by providing more informative kinetics comments and additional network output on errors.
- Prefixes estimated kinetics comments with the reaction family name.
- Catches
InvalidMicrocanonicalRateErrorto draw and save a network diagram before re-raising. - Logs a summary of the network when grain adjustments fail in pressure-dependence.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rmgpy/rmg/pdep.py | Added import of InvalidMicrocanonicalRateError and wrapped rate-coefficient calculation in a try/except that draws and logs the network diagram. |
| rmgpy/pdep/network.py | Called self.log_summary() before raising when grain count adjustments do not reduce the error. |
| rmgpy/data/kinetics/rules.py | Changed kinetics.comment strings to f-strings that include the family name and simplified concatenation. |
Comments suppressed due to low confidence (3)
rmgpy/rmg/pdep.py:881
- Add a unit test to trigger
InvalidMicrocanonicalRateErrorinupdate()so you can verify that the network is drawn, saved, and logged as intended before the exception is propagated.
except InvalidMicrocanonicalRateError:
rmgpy/rmg/pdep.py:882
- The variable
output_directoryis not defined in this scope, which will raise aNameErrorwhen the exception is caught. Consider passing it intoupdate()or using an existing attribute likeself.output_directory.
if output_directory:
rmgpy/data/kinetics/rules.py:370
- [nitpick] Since the comment text is parsed by downstream tools (per #2787), ensure the updated format (
in family …) is compatible with those parsers and update any related tests or parsing logic.
kinetics.comment = f"Estimated from node {entry.label} in family {self.label.replace('/rules','')}."
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 Passed Edge Comparison ✅Original model has 106 species.
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:02 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species.
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:00:58 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Passed Edge Comparison ✅Original model has 133 species.
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:01:31 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:51 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species.
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:02 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. Non-identical kinetics! ❌
kinetics:
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:00:59 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(Cyclopropene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics:
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:01:35 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
rwest
changed the title
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:49 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)(Cds-Cds)(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)(Cds-Cds)) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)H) + group(Cds-CdsCsH) + group(Cdd-CdsCds) + Estimated bicyclic component: polycyclic(s4_6_6_ane) - ring(Cyclohexane) - ring(Cyclohexane) + ring(124cyclohexatriene) + ring(1,4-Cyclohexadiene) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics:
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:01:55 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species.
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:00:58 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Passed Edge Comparison ✅Original model has 133 species.
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:01:30 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
rmgpy/rmg/pdep.py
Outdated
| K = self.calculate_rate_coefficients(Tlist, Plist, method) | ||
| except InvalidMicrocanonicalRateError: | ||
| if output_directory: | ||
| job.draw(output_directory, filename_stem=f'network{self.index:d}_{len(self.isomers)}', file_format='pdf') |
There was a problem hiding this comment.
It might make sense to pick a different name for this plot that's drawn during debugging. Instead of network1_1.py something like debug_network1_1.py so the user has a better clue that this is a debugging file that was generated as a result of an error,
There was a problem hiding this comment.
Thanks. I think I want to keep the name matching what it would have been called anyway, but have now modified the message printed in the log (if anyone reads the logs) to help the user.
| kinetics = deepcopy(entry.data) | ||
| if entry0 == entry: | ||
| kinetics.comment = "Estimated from node {}".format(entry.label) | ||
| kinetics.comment = f"Estimated from node {entry.label} in family {self.label.replace('/rules','')}." |
There was a problem hiding this comment.
This "in family ___" addition breaks the comment parser for uncertainty estimation.
One possible fix is a single line added between lines 4489 and 4490 of RMG-Py/rmgpy/data/kinetics/family.py:
template_str = template_str.split('in family')[0].strip() to break off the extra "in family ______"
The reason the comment parser is so fragile is that the chemkin writer currently breaks up lines longer than 150 characters, which makes it really hard to extract the node names from the autogenerated trees that look like Root_N-4R->H_N-4CNOS-u1_N-1R!H->O and are longer than 150 characters.
It currently assumes there's nothing between the node name and "Multiplied by reaction path degeneracy"
Also, until we have some sort of functional testing, the CI won't catch this kind of error: #2789 :(
There was a problem hiding this comment.
Thanks! Exactly the code review I was needing!
I've added the breaking phrase to the testing example, and added your suggested fix.
I like your issue #2789 - it's clearly put. Now we* just need to address it!
|
Aside from the broken comment parser issue, this looks good to me. Brief summary of the things I tested:
|
|
Thanks for the thorough review, and suggested fixes, @sevyharris . |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:49 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)(Cds-Cds)(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)(Cds-Cds)) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)H) + group(Cds-CdsCsH) + group(Cdd-CdsCds) + Estimated bicyclic component: polycyclic(s4_6_6_ane) - ring(Cyclohexane) - ring(Cyclohexane) + ring(124cyclohexatriene) + ring(1,4-Cyclohexadiene) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics:
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:01:56 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species.
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:02 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(Cyclopropene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics:
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:01:47 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
For auto-generated trees, the reaction family was not reported in the kinetics comment. Now it is. (Also replaced some format() calls with f-strings)
If a pdep network is going to cause the job to crash with an InvalidMicrocanonicalRateError, then report a summary of the troublesome network and draw a PDF of it before crashing, to make it easier to figure out what's wrong.
…ame_stem argument. Instead of it always being called network.pdf you can now change what it's called. Default is still network.pdf, and argument order is preserved if you call as ordered arguments, for backwards compatibility. This new feature is used when saving the diagram in the case of an InvalidMicrocanonicalRateError
You can set generatePESDiagrams=True in the options() block of an input file if you want to save them all. This is presumably a runtime penalty, or at least a hard disk space penalty, and probably shouldn't be a default. Default is False. Fixup: When the pdep_settings was made from an Arkane explorer job rather than an RMG run, the generate_PES_diagrams attribute does not exist. So we now assume False if not specified.
It doesn't actually _parse_ the new comment to figure out what family the thing comes from, but in most contexts it is not needed. At least now it doesn't crash. Also added the new phrase to at least one item in the testing examples so the unit tests could catch this. Co-authored-by: Richard West <[email protected]> Co-authored-by: Sevy Harris <[email protected]>
2 participants
Motivation or Problem
When debugging issues with pdep networks it's helpful to know where the rates came from and what the reaction network looks like.
Description of Changes
Estimated from node {}it saysEstimated from node {} in family {}InvalidMicrocanonicalRateErrorthen just before crashing it reports a summary of the network, draws a picture of it, and tells you where to find the picture.generatePESDiagramsoption in RMG input files to save potential energy surface diagrams for all pdep networks.Testing
These changes were helpful for me.
Reviewer Tips
Sometimes kinetics comments are read by machine not just human, so maybe we should look out for that when changing comments. But we recently increased the test coverage for that sort of thing (#2787) so that should make us a little more safe.
Other notes
Related to #2791 (comment). I'll fix that in a separate PR #2835.