-
Notifications
You must be signed in to change notification settings - Fork 13
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
Update ctests to include more obs types for more realistic testing #184
Update ctests to include more obs types for more realistic testing #184
Conversation
@ShunLiu-NOAA we better do CI tests after the new changes. I don't think there will be any issues but it is still good to retest after new changes. I will add "test_*" tags now and it should be completed in an hour. |
PASSED on jet started build_and_test on jet at UTC time: Fri Oct 11 17:06:53 UTC 2024
workdir: /lfs5/BMC/wrfruc/rrfsbot/PRs_RDASApp/184 |
PASSED on hera started build_and_test on hera at UTC time: Fri Oct 11 17:09:05 UTC 2024
workdir: /scratch1/NCEPDEV/fv3-cam/rrfsbot/PRs_RDASApp/184 |
PASSED on hercules started build_and_test on hercules at UTC time: Fri Oct 11 17:06:06 UTC 2024
workdir: /work/noaa/wrfruc/rrfsbot/PRs_RDASApp/184 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SamuelDegelia-NOAA for addressing some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SamuelDegelia-NOAA line 54:
file(COPY ${src_yaml}/${case}.yaml DESTINATION ${casedir} )
and other copying lines.
These copying/linking only runs at the build time.
This is sometime inconvenient in the develop/debug process as we have to track down to find what files to copy/link and then manually do it for new changes (to avoid unnecessary long re-building process). It will be good if we have a common script which can be used by the build process and at the same time we can also run it outside of the build process to facilitate debugging/developing.
I have figured this out for my rrfs-test eror and manually copied/linked new files. So it is not a high priority at the moment but it is good to have this capability. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my ctest developments, I normally just run build.sh
but add an exit command after the cmake
command. But yeah adding a short shell script similar to rrfs-test/CMakeLists.txt
could be a little easier. I'll do that at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be great! Thanks, @SamuelDegelia-NOAA!
This PR adds a simple shell script based on `rrfs-test/CMakeLists.txt` that allows developers to update the RRFS ctests in RDASApp. The goal here is to be able to run this quick script if making any changes to the yaml files, test reference files, or input data for the ctests. Previously, the only way to do this was to rebuild all of RDASApp or make temporary changes to `build.sh` Please see #184 (comment) for additional motivation. Note: this script cannot update the ctest configuration (number of tests, test names, or MPI configuration) since those options are handled by `ecbuild` during the cmake step.
This PR addresses #178 by updating the mpas-jedi ctests in RDASApp to now use a larger set of observations for more realistic testing. The yaml files for these new ctests are generated from the individual yamls added by other developers into
rrfs-test/validated_yamls/templates/obtype_config
. A script is run during build calledgen_yaml_ctest.sh
that concatenates these yamls, merges them with one of the ctest yamls inrrfs-test/validated_yamls/templates/basic_config
, and then moves those new files intorrfs-test/testinput
for use in the ctests.Currently included in the ctests are mesonet, aircraft, aircar, and ATMS obs.
Other notes
RDAS_DATA/fix/expr_data/mpas_2024052700/obs_ctest
. Thanks to @delippi for providing these new obs.rrfs-test/testinput_expr
and linked into theexpr
directory so as to not interupt any ongoing testing by other developersCtest output for a fresh clone and build on Hera:
Once we have some initial reviews, one of the code managers can add the tags for CI tests on each machine. Also, if this PR is approved, I will update the Wiki to show how to further extend these ctests by adding additional observation types in
rrfs-test/validated_yamls/templates/obtype_config
. It should be pretty easy now - all that needs to be done is to add the yaml intoobtype_config
, update thegen_yaml_ctest.sh
script, and then update the test reference data.