Fix broken examples #2824
Fix broken examples #2824
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Some notes so far, now that I've gone through all the examples:
|
There was a problem hiding this comment.
I like having more details, but it's a bit annoying that the False is no longer False but is a tuple which would evaluate to True when cast to a boolean. Now you can't use the function so easily inside if blocks etc.
How about returning the reason (as a string) if it's true, and False if it's false?
As long as someone doesn't do something like if fails_species_constraints(species) == True: then that should be safe?
But maybe this is ok.
There was a problem hiding this comment.
Maybe we can refactor it to "passes_species_constraint" which evaluates to either True or the reason for failing (str). Then we need to check for passes_species_constraint == True, and if not, the reason is provided.
It's a safer coding practice to explicitly state the equality check (for example if passes = True: rather than if passes).
There was a problem hiding this comment.
might want !electrochem and !surface_development as well as !surface ?
(possibly some other things in the recommended.py file)
There was a problem hiding this comment.
this example crashed previously, which is why !surface was added. AFAIK these others aren't problematic as their inclusion does not lead to a crash. but we can add this if you want to be preventative
|
Status update: at this point, the easiest problems have been fixed. The remaining ones are either related to codebase functionality or Julia/RMS stuff... |
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! ❌
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: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:58 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: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.
|
|
We probably will also want to write unit tests for codebase issues that weren't caught.
|
|
According to @whgreen the restart mechanism has been known-"not super functional" for a while. The Julia stuff was untested from the get-go because it wasn't compatible with |
Previously, failing a species constraint would not give the user any specific reason why the species failed the constraint check. This refactors the fails_species_constraint code so that it also returns a reason for the test failure. This required a small refactor to existing usages of the function.
…eem to be unhandled
jonwzheng
force-pushed
the
fix/examples
branch
from
9451a13 to
755a207
Compare
|
The aforementioned RMS/julia issues are outside the scope of what can be fixed in this PR, so I've marked this as ready for review. |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:50 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:01:57 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:02 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed 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:48 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.
|
|
@BonhyeokKoo @KnathanM could you guys please take a look? |
Motivation or Problem
Some examples have also fallen out of date.
The process of fixing the examples shows that the codebase could perhaps be improved or more clear, so this includes some code updates too.
Description of Changes
Sometimes the simulations are really long. If it appears to run fine and doesn't have any obvious errors, I didn't run to completion, but it should be OK. I denote these with "seems OK"
Since the number of examples is large, here I will only include comments if it was fixed. A checkmark just indicates that it was fine:
1,3-hexadieneAt the very end you getError: Invalid k(E) values computed for path reaction "[CH2]C(C=CCC)C([CH2])C=CCC(47) <=> [CH2]C=CC(CC)C([CH2])C=CCC(44)". Error: Increasing number of grains did not decrease error enough (Current badness: 0.8, previous 0.8). Something must be wrong with network PDepNetwork #13 rmgpy.exceptions.InvalidMicrocanonicalRateError: ('Invalid k(E) values computed for path reaction "[CH2]C(C=CCC)C([CH2])C=CCC(47) <=> [CH2]C=CC(CC)C([CH2])C=CCC(44)".', 6.254637800015352, 1.0)c3h4seems OKcatalysis/ch4_o2catalysis/methane_steamOSError: Couldn't find kinetics library /home/jonzheng/greengroup/RMG/RMG-database/input/kinetics/libraries/Surface/Deutschmann_Ni/reactions.pyThis is fixed by updating the library path.ch3no2CO2RRseems OKcommentedFixed by modifying the sampled datapoints in intervalrmgpy.exceptions.KineticsError: The master equation data needs more temperature and pressure data points than are placed into Chebyshev polynomial. Currently, the data has 2 temperatures and the polynomial is set to have 6. The data has 3 pressures and the polynomial is set ot have 4dieselseems OKe85Fixed by adding!surfacermgpy.exceptions.DatabaseError: Can't estimate thermo of vacant site. Should be in library (and should be 0).ethane-oxidationexternal_libraryseems OKgri_mech_rxn_libFixed. The library fails carbene species constraints. This accompanies an update to the species constraints code to be more descriptive.halogensseems OKheptane-eg5heptane-filterReactionsseems OKliquid_catseems OKliquid_phaseseems OKliquid_phase_constSPCseems OKmethylformateFixed by adding!surface, but another error pops up.AttributeError: 'NoneType' object has no attribute 'kinetics'. This second error should be fixed by Update incorrectGlarborg/C2database entry RMG-database#699. Original error:rmgpy/data/thermo.py", line 1572, in get_thermo_data_for_surface_species raise DatabaseError("Can't estimate thermo of vacant site. Should be in library (and should be 0).")minimalminimal_dynamicsminimal_mlCurrently broken as we await a patch forChempropfrom RMG 3.9 to 3.11minimal_multisurfminimal_restart_from_seedHad a problem with initiating kinetics family and libraries ofseed,edge_seedwhich aren't handled by the code.minimal_sensitivityFixed by adding!surfacefor error:Error: Error attempting to get thermo for species Ni_4 with structure 1 X u0 p0 c0.rmgpy.exceptions.DatabaseError: Can't estimate thermo of vacant site. Should be in library (and should be 0).minimal_stagedminimal_surfaceminimal_thermofilterMR_testFixed by modifying the sampled datapoints in intervalFile "rmgpy/kinetics/chebyshev.pyx", line 193, in rmgpy.kinetics.chebyshev.Chebyshev.fit_to_data rmgpy.exceptions.KineticsError: The master equation data needs more temperature and pressure data points than are placed into Chebyshev polynomial. Currently, the data has 2 temperatures and the polynomial is set to have 6. The data has 3 pressures and the polynomial is set ot have 4nox_transitory_edgeInitial error fixed by allowing singlet O2, which may form fromprimaryThermoLibrary. Initially fails withrmgpy.exceptions.ForbiddenStructureException: Species constraints forbids input species O2(S) RMG[...]Leads to new error from the Julia side:Error: Model core reactions: Error: Too many to print in detail File "/home/jonzheng/greengroup/RMG/RMG-Py/rmgpy/rmg/reactionmechanismsimulator_reactors.py", line 624, in generate_reactor react = Main.Reactor(domain, y0, (0.0, self.tf), p=p) File "/home/jonzheng/.julia/packages/PythonCall/L4cjh/src/JlWrap/any.jl", line 262, in __call__ return self._jl_callmethod($(pyjl_methodnum(pyjlany_call)), args, kwargs) juliacall.JuliaError: MethodError: no method matching +(::Vector{Float64}, ::Float64) For element-wise addition, use broadcasting with dot syntax: array .+ scalarpropane_branchingseems OKpruning_testrms_constant_VSEI_pure_ACNerror:rmgpy/rmg/main.py", line 990, in execute for val in item.reactants + item.products: File "/home/jonzheng/.julia/packages/PythonCall/L4cjh/src/JlWrap/any.jl", line 288, in __add__ return self._jl_callmethod($(pyjl_methodnum(pyjlany_op(+))), other) juliacall.JuliaError: DimensionMismatch: dimensions must match: a has dims (Base.OneTo(2),), b has dims (Base.OneTo(1),), mismatch at 1SEI_pure_ECsame as above.SR_testsuperminimalTEOSFixed by adding!surfacefor error:rmgpy.exceptions.DatabaseError: Can't estimate thermo of vacant site. Should be in library (and should be 0).Other
We may want to add some of these to our testing suite?