Expanding the unit testing of ring decomposition features. #2852
Conversation
These should all be bicyclic.
Test that the decomposition of a polyring into bicyclics and then into single rings is deterministic. This is important because the thermo estimation depends on the order of the rings. Currently this is not guaranteed, so if this test fails, we just skip it. See #2562
|
Which of these fail on the new SSSR approach? Are we failing to perceive bicycles, and/or failing to reproducibly decompose them? Based on the contents of the regression test you commented here it seems like the new SSSR correctly perceives bicycles, just decomposes them differently than |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:57 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: DetailsObservables 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:20 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. DetailsObservables 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:11 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. DetailsObservables 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:04 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables 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.
|
|
My guess is the "is bicyclic?" is probably failing because the symmetrized SSSR returns 3 rings instead of the strictly smallest SSSR of 2. Maybe we can merge this, rebase the other, then see about fixing? |
|
It's annoying regression tests come back "failing" with different entropies (I guess non deterministic symmetry numbers? Maybe we can finally fix this too!). But at least the enthalpies (and thermo comments) are unchanged. |
Seems like a good plan! 👍 |
Motivation or Problem
There's some debate in reviewing #2796 as to whether certain changes showing in the regression tests indicate that it is breaking things or improving things.
So I have made a couple of unit tests, that should pass.
Spoiler alert: they pass on
mainand not yet on #2796.Description of Changes
One is just adding a few bicyclic molecules to the
test_is_bicyclic1test.Another is adding a new test_deterministic_bicyclic_decomposition, that addresses issue #2562.
This one currently "skips" on main about 1/3 of the time, because we have a nondeterministic behavior, and can get one of two different ring decompositions. But the other parts of the test all pass.
See the commit messages and diffs for more details.