Skip to content
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

Perform a comprehensive set of thermodynamic consistency tests for all thermo models #1299

Merged
merged 40 commits into from
Jun 10, 2022

Conversation

speth
Copy link
Member

@speth speth commented May 31, 2022

Changes proposed in this pull request

  • Introduce a uniform set of thermodynamic consistency tests that can be performed for all thermo models with a minimal amount of code duplication, totaling nearly 2000 distinct unit tests.
  • Provide options for skipping certain tests for specific thermo models when there are known failures
  • Specific tests are skipped automatically if a method raises a NotImplementedError
  • Bump the version of Googletest used, which also bumps the minimum version of Visual Studio to 2017 (though the oldest version we can test on is 2019, due to a SCons bug).
  • Remove a number of now-redundant ad hoc tests of thermodynamic consistency

I've already uncovered several bugs using these tests, and have created issues for the ones that aren't being resolved as part of this PR (all of which tag this PR).

If applicable, fill in the issue number this pull request is fixing

Resolves Cantera/enhancements#114

If applicable, provide an example illustrating new features this pull request is introducing

All of the configuration for a set of consistency tests is specified in a YAML file, consistency-tests.yaml:

binary-solution-tabulated:
  setup:
    file: thermo-models.yaml
    phase: graphite-anode
    known-failures:
      g_eq_h_minus_Ts: Inconsistent results when P != 1 atm (states 2 and 3)
      g_eq_sum_gk_Xk: Inconsistent result when P != 1 atm (states 2 and 3)
  states:
  - {T: 300, P: 1 atm, X: {"Li[anode]": 0.3, "V[anode]": 0.7}}
  - {T: 320, P: 1 atm, X: {"Li[anode]": 0.3, "V[anode]": 0.7}}
  - {T: 300, P: 10 atm, X: {"Li[anode]": 0.6, "V[anode]": 0.4}}
  - {T: 300, P: 10 atm, X: {"Li[anode]": 0.0, "V[anode]": 1.0}}

And there is just a short block of code to create the parameterized test suite:

INSTANTIATE_TEST_SUITE_P(BinarySolutionTabulated, TestConsistency,
    testing::Combine(
        testing::Values(getSetup("binary-solution-tabulated")),
        testing::ValuesIn(getStates("binary-solution-tabulated")))
);

Which runs a set of different tests for each of the specified state vectors. Failing tests show the information about the particular case that is failing, e.g.:

[ RUN      ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/0
[       OK ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/0 (0 ms)
[ RUN      ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/1
[       OK ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/1 (0 ms)
[ RUN      ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/2
[       OK ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/2 (0 ms)
[ RUN      ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/3
test/thermo/consistency.cpp:118: Failure
The difference between phase->enthalpy_mole() and phase->mean_X(hk) is 7.232221142950948, which exceeds atol, where
phase->enthalpy_mole() evaluates to 13068.183201924798,
phase->mean_X(hk) evaluates to 13060.950980781847, and
atol evaluates to 1.0000000000000001e-05.
[  FAILED  ] BinarySolutionTabulated/TestConsistency.h_eq_sum_hk_Xk/3, where GetParam() = (file: thermo-models.yaml,
phase: graphite-anode, state: {T: 300, P: 10 atm, X: {Li[anode]: 0.0, V[anode]: 1.0}}) (0 ms)

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

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@speth ... this looks great. The only thing that I believe is needed are some comments documenting how this test works - the code is readable, but it may be daunting for anyone not sufficiently familiar with AnyMap and testing.

As a comment, I am also wondering whether google tests mind splitting things into *.h and *.cpp, as the actual tests only start on line 184 of consistency.cpp. If not, then some comments indicating of what users should add where would be good to have.

test/thermo/consistency.cpp Outdated Show resolved Hide resolved
test/thermo/consistency.cpp Outdated Show resolved Hide resolved
test/thermo/consistency.cpp Show resolved Hide resolved
@ischoegl
Copy link
Member

ischoegl commented Jun 3, 2022

About googletest: they now follow a Abseil live at head philosophy that won’t patch issues and recommends using the most recent commit. I am wondering whether this has something to do with the failures here?

@speth
Copy link
Member Author

speth commented Jun 3, 2022

I don't think the idea of "Abseil live at head" is that they won't patch issues, it's that they (mostly) won't backport patches to the previous version or make a bug fix release, and that if you want a version that has all the latest bug fixes, you should just run the latest commit to the main branch.

The current error is because MSVC++ 14.1 (Visual Studio 2017) doesn't seem to be available on the Github Actions builders -- only 14.0, 14.2, and 14.3, depending on whether it's the Windows 2019 or Windows 2022 builder.

@bryanwweber
Copy link
Member

@speth Is that the known problem with SCons rearing its head again?

@ischoegl
Copy link
Member

ischoegl commented Jun 3, 2022

@bryanwweber ... good point, although the failure on my windows box (which has all MSVC versions installed for VS2022) looks different - it fails much earlier Edit; I configured for less output, so it is the same ... I am just compiling main:

(cantera-dev) PS C:\Users\ischo\GitHub\cantera> scons build msvc_version=14.1
scons: Reading SConscript files ...
INFO: SCons 4.3.0 is using the following Python interpreter:
C:\Users\ischo\miniconda3\envs\cantera-dev\python.exe (Python 3.10)
Compiling with MSVC 14.1
Compiling for architecture: amd64
Compiling using the following toolchain(s): ['default']

scons: warning: VC version 14.1 not installed.  C/C++ compilers are most likely not set correctly.
 Installed versions are: ['14.3', '14.0']
File "C:\Users\ischo\GitHub\cantera\SConstruct", line 727, in <module>

scons: warning: VC version 14.1 not installed.  C/C++ compilers are most likely not set correctly.
 Installed versions are: ['14.3', '14.0']
File "C:\Users\ischo\GitHub\cantera\SConstruct", line 727, in <module>
INFO: Building Cantera from git commit '3d5de8222'
INFO: Configuration variables read from 'cantera.conf' and command line:
    msvc_version = '14.1'

INFO: Adding conda include and library paths: C:\Users\ischo\miniconda3\envs\cantera-dev
Checking for C++ header file cmath... no
ERROR: The C++ compiler is not correctly configured (incomplete include paths).
See 'config.log' for details.

Locally, I can only use 14.0 (oldest) or 14.3 (newest), with none of the others working. Fwiw, my SCons version is 4.3.0.

@ischoegl
Copy link
Member

ischoegl commented Jun 3, 2022

@bryanwweber / @speth ... hold on. It is the same issue 🎉

@bryanwweber
Copy link
Member

How much overlap is there between the phases you're identifying here and the list of undocumented and interested phases in the issue that I can't find because I'm on my phone? Are there any phases that we can remove?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Issues about tests, running the tests, or test results Thermo
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Improve testing of thermodynamic consistency
3 participants