RMG-Electrochem #2316
RMG-Electrochem #2316
Conversation
|
This pull request introduces 5 alerts when merging 10314f2 into 2a35d62 - view on LGTM.com new alerts:
|
|
This pull request introduces 5 alerts when merging 4f6b5db into e3d0617 - view on LGTM.com new alerts:
|
mjohnson541
force-pushed
the
electrochem
branch
from
982d5bc
to
9bf9529
Compare
mjohnson541
force-pushed
the
electrochem
branch
2 times, most recently
from
df1cfa5
to
408c5b5
Compare
A collection of Matt and David's commits from #2316, with updates and fixes added by Richard That pull request is too large to review, so some parts are being merged first.
|
I rebased, did a few interactive rebases to clean up some fixups and merge conflict markers that had gotten in there, and force pushed. |
|
The database tests are failing, because some sample molecules are being made that are charged. etc. looks like they're probably mostly the same cause. |
|
Thanks for the help! I meant to get on this last weekend, but I've been ill and now I'm frantically trying to get some urgent calculations done. Hopefully I'll be working on this PR as soon as those launch. At some point a month or so ago I think I figured out why that was happening, but I don't remember now. Skimming the sample molecule code doesn't seem to be jogging my memory. |
|
Going to try and introduce a workaround for this, some related issues elsewhere: |
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: #2316 (comment)
Regression Testing Resultscat: write error: Resource temporarily unavailable Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:32 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:03:10 liquid_oxidation Failed Core Comparison ❌Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 202 species. Non-identical thermo! ❌
thermo: Thermo library corrected for liquid phase: primaryThermoLibrary + Solvation correction with pentane as solvent and solute estimated using Solute library: hydrogen
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:02:11 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 132 species. 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: 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:34 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Failed Edge Comparison ❌Original model has 230 species. 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: 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: Non-identical kinetics! ❌
kinetics:
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:20 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:52 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:03:34 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:09:42 RMS_CSTR_liquid_oxidation Failed Core Comparison ❌Original model has 37 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 206 species. Non-identical thermo! ❌
thermo: Thermo library corrected for liquid phase: primaryThermoLibrary + Solvation correction with pentane as solvent and solute estimated using Solute library: hydrogen
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
The following observables did not match: ❌ Observable species CCCCC varied by more than 0.100 on average between old model pentane(2) and new model pentane(2) in condition 1.
RMS_CSTR_liquid_oxidation Failed Observable Testing ❌beep boop this comment was written by a bot 🤖 |
|
I think the differences in the core regressions for liquid runs look alright, the observable failure for RMS_CSTR_liquid_oxidation gave me a bit of a pause, but it looks like the observable for that isn't even for the same phase simulation as the run so I'm inclined to ignore it. |
|
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. |
|
@mjohnson541 could I jump in and help get the conflicts resolved with the testing files? I can't review the content but can at least make it merge-able. |
|
Sure! That'd be super helpful! I should be back working on merging this in a couple weeks. |
|
@mjohnson541 great! I will get the conflicts resolved before then, but probably by opening a PR into this branch from a separate branch so you can look at the changes. Do you have any idea of how 'close' this PR is to done (besides the conflicts)? |
|
Awesome! Thanks so much! Apart from the conflicts it should be ready for review and merging. |
I don't understand. What do you mean by that the observable is not even for the same phase simulation as the run? RMS_CSTER_liquid_oxidation is essentially liquid_oxidation except as open system. There is only liquid phase. If you're talking about the regression simulation being run at gas phase but not liquid phase, that is what liquid_oxidation does as well. I don't think we should ignore the RMS regression tests. |
In my opinion we should probably not in general be taking a model we built for liquid system A and then benchmarking it on gas phase system B. Philosophically, I think it's kind of like taking a fish out of water and measuring whether it climbs a tree as well as the last fish. Technically it tells you something about how different the fish is and a really bad fish is probably not going to be able to climb a tree well but it isn't what you optimized the fish for so it doesn't tell you if your fish is a better or worse fish in the water. Outside of that, while I didn't elaborate on it before, I also don't think it's that surprising to see the concentration change by more than 10% for pentane given the changes in species/reaction selection due to the new solute data and the limited size of the model. |
I agree that the current way of doing it is not ideal and someone should go in and implement a liquid phase version of regression run. But since the current stable version was run with gas phase, theoretically the PR'd version run in gas phase should at least be consistent. It just doesn't make sense to say it's ok to do such comparison for the liquid_oxidation case, but not ok for the RMS_CSTR_liquid_oxidation case. |
I'm confused, I don't think I ever said it was fine to do that in the liquid_oxidation case, but not the RMS_CSTR_liquid_oxidation case? In general it is fine for us to have regression tests that primarily exist for us to register if a change has occurred. However, in those cases I think we should concern ourselves with the causal changes in parameter estimation rather than the observable itself. |
|
Did this get stalled? I'm trying to interpret the comments above. Is someone resolving conflicts? |
Hi yes this is waiting on me. I will open a PR into this branch to get the tests fixed. |
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: #2316 (comment)
|
@mjohnson541 do you have any example input files for electrochemistry that you could include here? @JacksonBurns what's the status with #2598 ? do we rebase that onto this, and then both onto main? |
I had intended that one to help with the rebasing, but as I mention in a comment there it might not be 'easy' still... That branch is much closer to being compatible with |
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: #2316 (comment)
|
I have examples, but part of why they aren't on this branch yet is this isn't the complete PR, I didn't want to get in the way of the rebase, but I have the rest of the changes locally. I can try to get those added soon if we're ready. |
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: #2316 (comment)
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: ReactionMechanismGenerator#2316 (comment)
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: #2316 (comment)
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: #2316 (comment)
this will allow the regression test results to all be reported even if there is such a huge difference between the dynamic and baseline that the system utilities cannot print it see: #2316 (comment)
Enables RMG to operate on solid electrolyte interface conditions in Li-ion batteries. Adds Li atoms, adds electrochemical reactions, adds a liquid-catalysis reactor and upgrades tree generation adding corrections for kinetic solvent effect.