Big rework of P3 around new BFB baseline unit tests #3096
Conversation
jgfouca
requested review from
bartgol,
ambrad,
PeterCaldwell,
AaronDonahue,
tcclevenger and
mahf708
|
This is great @jgfouca! Thanks for putting this together. |
|
@AaronDonahue , thanks! My fingers are bleeding but I'm quite happy with the result. |
| @@ -26,4 +26,21 @@ PhysicsTestData& PhysicsTestData::assignment_impl(const PhysicsTestData& rhs) | |||
| return *this; | |||
| } | |||
|
|
|||
| void PhysicsTestData::read(const ekat::FILEPtr& fid) | |||
There was a problem hiding this comment.
At some point we should get rid of the ekat FILEPtr data structure and work directly with fstream from the std library...
|
@bartgol , yes. You can look at the change list in the PR description for the summation of what changed because this PR is probably too big to review in the source code. |
Do you think there would be added value in doing
? It would only make us change rand seed when we regen baselines, but maybe on the long run it may help uncover hidden bugs? |
|
@jgfouca just to make sure you noticed it: the FPE build fails, and since that should have no baselines tests, it means something is really off. |
|
@bartgol ,
That's a really good idea. |
| scream::p3::P3GlobalForFortran::deinit(); | ||
| } | ||
|
|
||
| std::mt19937_64 get_engine() |
There was a problem hiding this comment.
@bartgol , I added the feature you wanted here.
| if (SCREAM_ENABLE_BASELINE_TESTS) | ||
| CreateUnitTest (p3_sk_tests_fail p3_rain_sed_unit_tests.cpp | ||
| LIBS p3_sk p3_test_infra | ||
| EXE_ARGS "--flags=\\'${BASELINE_FILE_ARG}\\'" |
There was a problem hiding this comment.
I am not sure this is the way --flags is to be used in ekat's test main. This is meant to be used for stuff like "verbose", and is meant to set a "bool" in the test session. Here, you are trying to pass actual executable flags, but catch2 will likely complain about them, since they are not recognized.
There was a problem hiding this comment.
For run_and_cmp, this works b/c it uses its own main. Here, we need to work around it.
- For boolean, you can use stuff like
--flags generate,verbose, which will set"verbose"and"generate"as valid keys in the map string->bool in the test session - For args that are not boolean, we need to use
--ekat-test-params(which I want to sometime shorten to--params), and the syntax could be--ekat-test-params baseline-dir=${SCREAM_BASELINES_DIR}/data.
There was a problem hiding this comment.
I tried a simple program that prints the test session flags/params:
std::cout << "flags:\n";
for (auto it : ts.flags) {
std::cout << it.first << " -> " << it.second << "\n";
}
std::cout << "params:\n";
for (auto it : ts.params) {
std::cout << it.first << " -> " << it.second << "\n";
}
std::cout << "DONE!\n";I got
$ ./test --flags='-c -b /path/to/something'
Calling initialize_kokkos
ExecSpace name: Serial
ExecSpace initialized: yes
active avx set: -AVX512F-AVX2-AVX
compiler id: GCC
FPE support is disabled
#host threads: 1
Starting catch session on rank 0 out of 1
flags:
-c -b /path/to/something -> 1
params:
DONE!There was a problem hiding this comment.
The core pb is that catch2 does not let you pass arbitrary flags. So I had to come up with the whole --flags f1,f2,f3 --ekat-test-params p1=v1,p2=v2,p3=v3 syntax...
…_f90 * origin/master: (64 commits) EAMxx: adapt to changes in ekat's catch main logic Update ekat submodule Update .mergify.yml EAMxx: fix query-scream util script Update .mergify.yml Workflows: nano-fixes to eamxx workflows Mergify: fix syntax ci(Mergify): configuration update Workflows: reworked how to skip eamxx testing jobs Workflows: added more eamxx workflows Workflows: fix logic for eamxx jobs skipping Workflows: allow to generate ALL baselines at ones for eamxx EAMxx: Use the main branch of mam4xx. EAMxx: Add tracer_reader_utils.hpp to microphysics.hpp. EAMxx: Remove temporary CPP macro. EAMxx: Update description of dry aerosol particle diameters. EAMxx: Use a temporary branch for mam4xx to test changes in the photo table code. EAMxx: Move read utils files to readfiles folder. EAMxx: Refine linoz parameter handling and class member references Correcting data types for variables, refining comments, and updating YAML input configurations. ...
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 Enforce checks passingThis rule is failing.Make sure that checks are not failing on the PR, and reviewers approved
|
|
All the fails are expected due to missing baselines. |
This turned into a huge PR.
Change list:
Follow on work:
Notes: