-
Notifications
You must be signed in to change notification settings - Fork 18
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
Unify obs and test configurations between FV3-JEDI and MPAS-JEDI #255
base: develop
Are you sure you want to change the base?
Unify obs and test configurations between FV3-JEDI and MPAS-JEDI #255
Conversation
Ctest results from Hera:
|
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.
This PR is quite large. It's generally better to keep PRs smaller when possible to make reviews easier. One of my main concerns is with the large yaml files under testinput
. Are those files truly necessary? It seems like you already have templates in place, which could significantly reduce the amount of committed code. In a sense, committing unnecessarily long files is similar to committing binaries--every update to these files creates a new snapshot that github must store. Over time, this can increase repository size and impact the time required for cloning and pulling.
rrfs-test/validated_yamls/templates/basic_config/fv3jedi_en3dvar.yaml
Outdated
Show resolved
Hide resolved
I agree that this PR is very large - sorry about that! I just kept finding little things that needed to be changed to match the configuration between GSI/FV3-JEDI/MPAS-JEDI. I kept adding those changes to the PR but I probably should have stopped somewhere and made individual PRs. One thing I struggled with is that the test references have to be updated for each change in this PR, so if we made individual PRs then there could be conflicts. Also, updating the mpi tasks for each ctest also meant rerunning bump for each solver. So it made sense to include localization changes at the same time. But still, I agree that this became too large. Would it still be helpful to go ahead and split this up? I could have individual PRs for
|
Also, in regards to commiting the super yamls, this is something we discussed in #184 and #187. It was suggested to commit the super yamls into the repo (instead of creating them at runtime or during the build) so that other developers can use them as templates. But I agree that as these yamls become larger, that PRs like this will have lots of changes to them which make it very hard to review. These changes would also show up in both the templates and the super yamls. I am not fully sure the solution here, but maybe we should revisit the choice to commit the super yamls. Tagging @guoqing-noaa and @ShunLiu-NOAA here for potential thoughts. |
rrfs-test/testinput_expr/rrfs_mpasjedi_2024052700_Ens3Dvar.yaml
Outdated
Show resolved
Hide resolved
I’d like to revisit this discussion, please, as I may have missed it earlier. I don’t think we should commit any of the large "super" yamls to RDASApp or JCB. We should only maintain a single copy of each file to keep the repository manageable. Committing these large files makes the PR unnecessarily large and harder to maintain and prone error. If the argument for including them is that they can serve as templates for other developers, that doesn’t make sense. Developers should reference the Additionally, relying on these super yamls in ctest introduces a risk of missing updates or using outdated versions, which can cause issues down the line. For example, if the super yamls continue using "DRIPCG," that outdated configuration could propagate to other PRs and test results, perpetuating the problem. |
I would like to make a clarification that GIT treats text files and binary files very differently. For binary files, GIT cannot track delta changes. It can only save a complete copy for different commits. That will increase a repo size quickly. For text files, GIT uses a |
@guoqing-noaa thanks for adding that clarification. That makes sense that binaries are treated different in that way. I think there are valid reasons for doing this either way and I hope we can all discuss and come to some consensus on how we should handle this. I would say that my main concern is that we should just keep one copy of each yaml part vs having as many as 9 different copies of the same thing that you have to maintain. |
…file in IODA/yaml/prepbufr_msonet.yaml
I'll add a bit to the discussion about committing the super yamls. If we commit these files, it can make PRs like this one hard to review as we see lots of extra changes show up. For example, adding ATMS obs back to the ctests for FV3-JEDI caused lots of big changes to However, I just realized that there is a downside to creating the super yamls during At this point, I am thinking that it might be worth it to just leave the super yamls as they are in the repo and not dynamically update them for the ctests. We wanted to increase the number of obs for the ctests for more realistic testing, but at this point these yamls already have a large number of obs and are now useful for that purpose. So maybe we can just keep the ctests as is and only update them and their associated super yamls if there are any updates to the test cases itself (and not the obs space). |
Description
The goal of this PR is to unify the obs and configuration between FV3-JEDI and MPAS-JEDI. New obs are generated by running the offline domain check with the FV3-JEDI grid (the smaller of the two grids). These new obs are staged in
$RDAS_DATA/fix/expr_data/obs_2024052700
and linked into the fix directories for both test cases.I also removed the older
obs_ctest
directory since it was confusing to have separate directories for the full obs and the obs used for the ctest. A lot of those files actually overlapped anyways. Now there is just oneobs
directory that should be the same for both test cases.A lot of other little things are included in this PR to better unify the configuration between the two test cases and GSI:
Data/fieldmetadata/tlei-gfs-restart.yaml
.niter
andgradient norm reduction
are now consistent between both cases for the Ens3Dvar test.&analysisDate
variable is now set correctly for FV3-JEDI to better match wind obs counts compared to MPAS-JEDI (important for the temporal thinning filter).RDASApp/rrfs-test/gsi_fix
.Lastly, a small utility is also added called
rrfs-test/ush/print_ctest_runtime.py
that can be used for printing the actual runtime of the ctests (instead of the runtime + wait time included in the normal ctest output).Issue(s) addressed
#246
Dependencies (if applicable)
List the other PRs that this PR is dependent on:
None
Checklist