-
Notifications
You must be signed in to change notification settings - Fork 240
Remove incorrectly calculated and applied energy_correction from pdep.py #2791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
After finding many submerged barriers when running RMG in pdep mode, I discovered that the energy_correction is calculated incorrectly *and* only applied to the transition state. The code calculates the energy_correction by averaging all species E0s. However, it should average the total energy for each stationary point (some of which are bimolecular), and I believe it would be more intuitive to use the minimum energy on the surface. A simple solution would be to remove the energy_correction altogether (what I've done here for now). If it is desired, we need to apply energy_correction to all stationary points, not just transition states, so that barriers are not submerged. The energy_correction could be calculated as follows... ``` stationary_point_E0s = [sum([spec.conformer.E0.value_si for spec in stationary_point]) for stationary_point in self.reactants + self.isomers + self.products])] energy_correction = -np.array(stationary_point_E0s).min() ``` I would need to track down where to apply it to the wells though.
Regression Testing ResultsWARNING:root:Initial mole fractions do not sum to one; normalizing. Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:09 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments:
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:22 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:01:28 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:02:29 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 ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:00:56 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
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:39 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Passed Edge Comparison ✅Original model has 18 species. Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:02:30 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
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:06:25 RMS_CSTR_liquid_oxidation Passed Core Comparison ✅Original model has 37 species. RMS_CSTR_liquid_oxidation Passed Edge Comparison ✅Original model has 248 species.
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:42 fragment Passed Core Comparison ✅Original model has 10 species. fragment Passed Edge Comparison ✅Original model has 33 species.
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:09 RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅Original model has 10 species. RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅Original model has 27 species.
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:45 minimal_surface Passed Core Comparison ✅Original model has 11 species. minimal_surface Passed Edge Comparison ✅Original model has 38 species.
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 🤖 |
@JacksonBurns @alongd @mjohnson541 would you be willing to review this PR? It is rather straightforward if we are willing to not apply |
My understanding is that the intent of this correction is exclusively so our energy diagrams are in the vicinity of zero. The choice of the correction is arbitrary, but intended to adjust the y-axis location in energy diagrams while keeping relative energies the same. Generally, I hate having this done in the vicinity of legitimate physical calculations and I think if we're going to shift the diagram we should do that when we draw the diagram. However, historically other people have had much stronger opinions about our energy diagrams than I do. So I would wait for @alongd's opinion. I don't mind getting rid of it altogether, but adding the species energy correction back in might be the easiest way to restore the desired functionality. |
Oh, boy, now I feel bad. I came across this problem (I think) back in December, and was half way to opening an issue and/or pull request, then got distracted. I was debugging a PDep network and as commonly done I we would find the ![]() I got about as far as @donerancl in realizing it's the inconsistently applied "correction". I think I determined that during running of the RMG job, the correction was correctly applied, but it was inconsistently (and incorrectly) saved in the arkane input files. The conformers would have the correction applied, but not the TS, or something like that. I kept the PDFs and windows and tabs open for about a month hoping to get around to finishing my investigation, but eventually had to reboot the laptop. I have one more stashed commit from February when I changed branch to do something else, where around line 26 of
I can't remember if that fixed it. Based on when David (I think) added the correction, I seem to recall it helped numerical issues (you end up taking exponentials of this energy when evaluating a partition function?) and was not just to make the PES figures look nice. |
Would this be a good solution?
I'm slightly worried that point 2 would be prone to bugs. I haven't looked. |
I think that's definitely the most readable way to handle this, but it does require identifying all locations where this might happen and some of the algorithmic fixes may be much more than couple line fixes. I suspect we have object stored vectors that may become 0/Inf/NaN without the correction which may require more involved refactoring. It would be good to fix that, but given the benefits/work cost perhaps its most practical now to ensure the correction is applied to all the energies correctly and add some detailed comments explaining what is going on. |
@rwest @mjohnson541 Thanks for your discussion -- just returned from vacation. I think you are right that we need to find everywhere the Also, somewhat related, I am noticing that many of the networks from the |
notes for my reference on places where
in
in
in
|
After finding many submerged barriers when running RMG in pdep mode, I discovered that the
energy_correction
is calculated incorrectly and only applied to the transition state. The code calculates theenergy_correction
by averaging all species E0s. However, it should average the total energy for each stationary point (some of which are bimolecular), and I believe it would be more intuitive to use the minimum energy on the surface. A simple solution would be to remove theenergy_correction
altogether (what I've done here for now). If it is desired, we need to applyenergy_correction
to all stationary points, not just transition states, so that barriers are not submerged.The
energy_correction
could be calculated as follows...I would need to track down where to apply it to the wells though.
Motivation or Problem
A clear and concise description of what what you're trying to fix or improve. Please reference any issues that this addresses.
Description of Changes
A clear and concise description of what what you've changed or added.
Testing
A clear and concise description of testing that you've done or plan to do.
Reviewer Tips
Suggestions for verifying that this PR works or other notes for the reviewer.