Migrate "test_problems" tests to use YAML input files by speth · Pull Request #1062 · Cantera/cantera

speth · 2021-06-16T02:59:56Z

In preparation for the removal of the CTI and XML formats, this updates all of the tests in the test_problems directory to use the YAML format (with the exception of a few tests that are run for each input format).

Changes proposed in this pull request

Update all test_problems tests to use YAML input
Fix a handful of small issues discovered while making this transition
- Use InputFileError to improve error messages related to sticking reactions
- Fix the phase dimension checks in newReaction to always use the correct phase
- Fix CTI to YAML conversion for files with no reactions
- Fix XML to YAML conversion of HMW phases with an implicit temperature model
- Handle the case of a missing reactionArray node in XML files
- Update the phase name aliases handled by ctml2yaml
Remove the surfkin and silane_equil tests, which weren't adding anything that wasn't better covered by other tests.

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

The relevant phase for determining whether or not a reaction occurs on an interface is not necessarily the first phase in the Kinetics object.

Set the 'reactions' field of the YAML phase definition for such input files to 'none'. Modify co2_RK_example.* input files so that they can be used for testing.

This makes it possible to define the phase list in-line when calling newKinetics.

Retain runs with the existing CTI and XML input files until those formats are removed.

Includes tests for classes IdealSolidSolnPhase and IdealSolnGasVPSS

Compare to the list of aliases defined in the ThermoFactory constructor

This test used only the the most trivial interface kinetics functionality, which is more thoroughly covered by other tests such as surfSolver in C++ and the TestReaction.test_interface and TestSofcKinetics in Python.

Basic gas-phase equilibrium is more thoroughly exercised by the GTest and Python test suites.

codecov · 2021-06-16T03:14:17Z

Codecov Report

Merging #1062 (cfda69a) into main (9de22e8) will increase coverage by 0.11%.
The diff coverage is 92.79%.

@@            Coverage Diff             @@
##             main    #1062      +/-   ##
==========================================
+ Coverage   72.92%   73.04%   +0.11%     
==========================================
  Files         357      357              
  Lines       47120    46925     -195     
==========================================
- Hits        34363    34277      -86     
+ Misses      12757    12648     -109

Impacted Files	Coverage Δ
include/cantera/kinetics/KineticsFactory.h	`100.00% <ø> (ø)`
src/kinetics/InterfaceKinetics.cpp	`85.71% <0.00%> (ø)`
test_problems/mixGasTransport/mixGasTransport.cpp	`96.75% <ø> (-0.03%)`	⬇️
...t_problems/multiGasTransport/multiGasTransport.cpp	`96.98% <ø> (-0.02%)`	⬇️
...est_problems/VCSnonideal/NaCl_equil/nacl_equil.cpp	`65.51% <62.50%> (+0.40%)`	⬆️
test_problems/diamondSurf/runDiamond.cpp	`94.80% <95.65%> (-0.32%)`	⬇️
src/kinetics/KineticsFactory.cpp	`96.80% <100.00%> (ø)`
src/kinetics/ReactionFactory.cpp	`97.46% <100.00%> (+0.01%)`	⬆️
test/kinetics/kineticsFromScratch.cpp	`100.00% <100.00%> (ø)`
test/kinetics/kineticsFromYaml.cpp	`98.34% <100.00%> (-0.04%)`	⬇️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9de22e8...cfda69a. Read the comment docs.

test_problems/surfSolverTest/surfaceSolver.cpp

test_problems/surfSolverTest/surfaceSolver2.cpp

ischoegl

Thanks for taking this on - this takes the transition from CTI/XML to YAML a big step forward. I took a cursory look at the changes, and most of them are mainly replacing input files (i.e. cumbersome, but straight-forward).

I do have two comments, though: documentation on some of the tests is relatively sparse. While it's certainly beyond the scope of this PR to make major changes, leaving very brief statements at the top may be helpful in the future (you are probably the one person who has the best overview).

There was a comment about Interface in one of the test problems - this is obviously deprecated in 2.5 and currently removed in 2.6. There were also recent questions about this on the UG. While I am not advocating on a reversal of this particular deprecation, do you see a way to introduce a new class that represents a collection of multiple C++ SolutionBase objects? (I don't think it's needed, but an inherited class that simplifies loading may be nice).

speth · 2021-06-17T01:10:17Z

I do have two comments, though: documentation on some of the tests is relatively sparse. While it's certainly beyond the scope of this PR to make major changes, leaving very brief statements at the top may be helpful in the future (you are probably the one person who has the best overview).

Poor/incorrect documentation is one of many faults of the test_problems test suite. Ultimately, I think it should be eliminated in favor of using the more explicit and expressive GTest and Python test suites. Progress towards this end has been very slow, though, in part because it's laborious, and in part because it's not always easy to make sure that the replacement is actually catching all the important behavior from the original test. I can create a catch-all issue to document some of the things that ought to be done and track progress, as in #267, but I don't think trying to document these tests is a very good use of time.

ischoegl · 2021-06-17T02:23:02Z

… I don't think trying to document these tests is a very good use of time.

Fair point. I think getting the Interfaces issue worked out is certainly much more impactful. Your enhancement post is really appreciated.

ischoegl

Overall, those changes all look good to me. The only things I am not absolutely sure about are the surface solver tests where you ”removed unused tests” (ed00efe), with associated code removals. If these indeed weren’t used at all, I’m ok to approve this PR.

On the other hand, there’s Cantera/enhancements#103, where a resolution may simplify some of the work here. (But I’m not a fan of leaving large PR’s lingering).

speth · 2021-06-17T02:56:38Z

The only things I am not absolutely sure about are the surface solver tests where you ”removed unused tests” (ed00efe), with associated code removals. If these indeed weren’t used at all, I’m ok to approve this PR.

The code removed in that commit falls into two categories:

Messy code for processing command line options that are never passed in as part of the test suite.
Code that is in unreachable branches, particularly where the variable ProblemNumber is hardcoded and then there are different if conditions based on the value of ProblemNumber, as well as some other simplifications that resulted from removing those bits of dead code.

You can tell that these changes didn't have an impact on what's actually tested from the fact that the "blessed" output files for these tests haven't changed.

ischoegl

Poor/incorrect documentation is one of many faults of the test_problems test suite. Ultimately, I think it should be eliminated in favor of using the more explicit and expressive GTest and Python test suites.

I agree that it would be preferable to eliminate those tests long-term. In the meantime, I am approving these changes.

bryanwweber · 2021-06-19T20:22:13Z

@ischoegl If you approve on behalf of @Cantera/committers then feel free to merge after ~24-48 hours from PR creation.

speth added 21 commits June 15, 2021 22:09

[Kinetics] Improve error messages related to sticking reactions

e2766e2

[Kinetics] Fix checking phase dimension in newReaction

08fced4

The relevant phase for determining whether or not a reaction occurs on an interface is not necessarily the first phase in the Kinetics object.

[Test] Replace CTI with YAML in surfSolverTest

68eaf4a

[Test] Remove unused cases from surfSolver tests

ed00efe

[Input] Fix conversion of CTI files with no reactions

17716d3

Set the 'reactions' field of the YAML phase definition for such input files to 'none'. Modify co2_RK_example.* input files so that they can be used for testing.

[Test] Replace CTI file used in ChemEquil_ionizedGas test

a78a11b

[Kinetics] Make phase list argument to newKinetics const

dca02be

This makes it possible to define the phase list in-line when calling newKinetics.

[Test] Update runDiamond test to use YAML

5aeeb8b

Retain runs with the existing CTI and XML input files until those formats are removed.

[Test] Re-enable CpJump test and convert to YAML

20e21e2

[Test] Update DH_graph tests to use YAML inputs

f7debea

[ctml2yaml] Fix conversion of HMW phase with implicit TempModel

2addb6c

[Test] Use YAML input for HMWSoln tests

0f2f8bb

[ctml2yaml] Treat missing reaction array as empty

f6384cc

[Test] Use YAML input in stoichSolidKinetics test

aad79ce

[Test] Use YAML in LatticeSolid equilibrium test

b35a16e

[Test] Use YAML input for ideal solid solution tests

4bd295d

Includes tests for classes IdealSolidSolnPhase and IdealSolnGasVPSS

[ctml2yaml] Add missing phase name aliases

0da3f95

Compare to the list of aliases defined in the ThermoFactory constructor

[Test] Use YAML input for IdealMolalSoln test

a165a12

[Test] Remove unnecessary 'surfkin' test

19ef5bc

This test used only the the most trivial interface kinetics functionality, which is more thoroughly covered by other tests such as surfSolver in C++ and the TestReaction.test_interface and TestSofcKinetics in Python.

[Test] Remove silane_equil test

876094f

Basic gas-phase equilibrium is more thoroughly exercised by the GTest and Python test suites.

[Test] Replace XML with YAML input for additional test_problems tests

cfda69a

ischoegl reviewed Jun 16, 2021

View reviewed changes

test_problems/surfSolverTest/surfaceSolver.cpp Show resolved Hide resolved

ischoegl reviewed Jun 16, 2021

View reviewed changes

test_problems/surfSolverTest/surfaceSolver2.cpp Show resolved Hide resolved

ischoegl reviewed Jun 16, 2021

View reviewed changes

speth mentioned this pull request Jun 17, 2021

Simpler way to create objects defining an interface Cantera/enhancements#103

Closed

ischoegl reviewed Jun 17, 2021

View reviewed changes

ischoegl approved these changes Jun 17, 2021

View reviewed changes

ischoegl merged commit e0b3823 into Cantera:main Jun 19, 2021

speth deleted the replace-cti-xml-test_problems branch June 25, 2021 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrate "test_problems" tests to use YAML input files#1062

Migrate "test_problems" tests to use YAML input files#1062
ischoegl merged 21 commits intoCantera:mainfrom
speth:replace-cti-xml-test_problems

speth commented Jun 16, 2021 •

edited by ischoegl

Loading

Uh oh!

codecov bot commented Jun 16, 2021

Uh oh!

Uh oh!

Uh oh!

ischoegl left a comment •

edited

Loading

Uh oh!

speth commented Jun 17, 2021

Uh oh!

ischoegl commented Jun 17, 2021

Uh oh!

ischoegl left a comment •

edited

Loading

Uh oh!

speth commented Jun 17, 2021

Uh oh!

ischoegl left a comment

Uh oh!

bryanwweber commented Jun 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

speth commented Jun 16, 2021 • edited by ischoegl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 16, 2021

Codecov Report

Uh oh!

Uh oh!

Uh oh!

ischoegl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

speth commented Jun 17, 2021

Uh oh!

ischoegl commented Jun 17, 2021

Uh oh!

ischoegl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

speth commented Jun 17, 2021

Uh oh!

ischoegl left a comment

Choose a reason for hiding this comment

Uh oh!

bryanwweber commented Jun 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

speth commented Jun 16, 2021 •

edited by ischoegl

Loading

ischoegl left a comment •

edited

Loading

ischoegl left a comment •

edited

Loading