-
Notifications
You must be signed in to change notification settings - Fork 65
Update main from develop 2025/07/24 --> 2025/08/21 #668
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
Conversation
… bugfix/scheme_ordering_in_caps
…in_caps Capgen in SCM: Bug fix for scheme ordering in non-run phases
Description: Allow for multiple instances of the same local_name being used in the Group cap for two situations: a) with different standard_names b) in different DDTs . User interface changes?: No Fixes: NCAR#629 Testing: Added to var_compatibility_test to exercise feature. This PR contains changes included in NCAR#630 --------- Co-authored-by: Dom Heinzeller <[email protected]>
Overview This PR contains changes to fix scheme subcycling in Capgen and extends the var_compatability_test to exercise subcycling. UPDATE: Added bugfix for suite-part list ordering (see change to ccpp_suite.py). Description Create local group variable for subcycle indexing. Fix bug (`self.loop` -> `self._loop`) in the Subcycle write phase, and in ccpp_datafile. User interface changes?: No Fixes: NCAR#632 Fixes: NCAR#634 Testing: Added to var_compatibility_test to exercise feature. --------- Co-authored-by: Dom Heinzeller <[email protected]> Co-authored-by: Steve Goldhaber <[email protected]>
Implement constituent index lookup routines
- Created a new CCPP source file, src/ccpp_scheme_utils.F90
- This file contains interfaces available to CCPP schemes
- It contains a `private` pointer to the CCPP constituent object so many
more interfaces could easily be created
- Implemented two interface routines to find constituent indices from
standard names
- ccpp_constituent_index: Lookup index constituent by name
- ccpp_constituent_indices: Lookup indices of consitutents by name
- Add tests of this functionality into advection_test
- Minor code cleanup
User interface changes?: Yes
- New routines available to CCPP schemes
Addresses NCAR#597
Testing:
test removed: None
unit tests: PASS
system tests: Added functionality to advection test, PASS
manual testing: NA
---------
Co-authored-by: Steve Goldhaber <[email protected]>
Co-authored-by: Steve Goldhaber <[email protected]>
Add options for pre-registered DDT names Add an option for pre-registered DDT names Add `ccpp_constituent_prop_ptr_t` as a default (pre-registered) DDT Add test for 'unknown' DDT variables User interface changes?: Yes Added a new option, `--ddt-names`, to allow users to simply add DDT names that allow generation of a CCPP metadata template from a Fortran source. Because it is an option, it is backwards compatible. Fixes: NCAR#643 ccpp_fortran_to_metadata.py errors out when given a file that has a constituent properties object Testing: test removed: None unit tests: NA system tests: NA manual testing: Added a manual test that tests this bug fix --------- Co-authored-by: Steve Goldhaber <[email protected]>
All in the title. This is the `CMakeLists.txt` update that was merged into main earlier today User interface changes?: No Fixes: Cleanup Testing: all tests pass with UFS and in CI test removed: n/a unit tests: pass system tests: pass manual testing: pass --------- Co-authored-by: Michael Kavulich <[email protected]> Co-authored-by: Grant Firl <[email protected]>
New implementation of differing module name Implement a solution that allows a Fortran module name to differ from the filename. An optional module_name keyword is added to the ccpp-table-properties (MetadataTable) section that allows the user to specify a Fortran module name that is independent from the name of the enclosing file.
…CAR#656) This PR adds logic to support equivalent and identical units in both capgen and ccpp-prebuild. 1. Support for identical units (`m2 s-2` is identical to `m+2 s-2`). Internally, both `capgen` and `ccpp-prebuild` convert the former to the latter. For `capgen`, this happens deep down in the unit conversion logic. If `capgen` finds that two units are identical after parsing them, it returns a `(None, None)` unit conversion that tells the VarCompatibilityObject that the variables are equivalent w.r.t. units. For `ccpp-prebuild`, this happens when translating `capgen` metadata to `ccpp-prebuild` metadata. The outcome is the same. 2. Support for equivalent units (`m2 s-2` is equivalent to `J kg-1`). This is handled differently for the two code generators. While `capgen` skips the unit conversion step (VarCompatObj says variables are equivalent, hence no conversion required), `ccpp-prebuild` uses an identity transformation `{var} = {var}`. As a result, the `units` metadata can now be written as `m2 s-2` or `m+2 s-2`. Both are acceptable and dealt with by the two code generators. The `ccpp-prebuild` implementation is simpler than the `capgen` implementation, because of the interface between the `capgen`-provided metadata from the parser and the metadata used by `ccpp-prebuild`, and also because the `ccpp-prebuild` implementation is more of a workaround (yes, prebuild needs to go). User interface changes?: No Fixes NCAR#570 Testing: test removed: none unit tests: all pass, including doc tests; added several doc tests for the new capabilities system tests: all pass; extended `capgen` and `ccpp-prebuild` tests to cover the new features manual testing: ran all unit and system tests on my laptop --------- Co-authored-by: Dustin Swales <[email protected]> Co-authored-by: Michael Kavulich <[email protected]> Co-authored-by: Test User <[email protected]>
This PR contains changes for the DEBUG tests. Add functionality within the DEBUG checks to: - Allow case-insensitivity for standard_names within dimensions. - Permit numerical dimensions in metadata. - Skip certain debug checks for arrays with non-standard lower-bounds - Remove tests for character and type arrays (not working) - Capgen tests updated to test new capabilities. User interface changes?: No Fixes: NCAR#649 NCAR#650 NCAR#651 NCAR#652 --------- Co-authored-by: Dom Heinzeller <[email protected]>
… host and schemes (NCAR#659) Detect invalid horizontal dimensions (loop variables) in metadata for host and schemes Detect invalid horizontal dimensions (loop variables) in metadata for host and schemes. For schemes, `horizontal_dimension` is invalid in the `run` phase, all other options (`horizontal_loop_extent`, `horizontal_loop_begin`, `horizontal_loop_end`) are only valid in the `run` phase. For host models, only `horizontal_dimension` is valid. Tests are added for both scheme and host metadata, and existing tests are fixed, namely bad horizontal dimensions in: - `test/unit_tests/sample_files/test_bad_var_property_name.meta` - `test/unit_tests/sample_files/test_multi_ccpp_arg_tables.meta` - `test/capgen_test/make_ddt.meta` - `test/capgen_test/temp_set.meta` - `test/ddthost_test/make_ddt.meta` - `test_prebuild/test_tracked_data/scheme3.meta` - `test_prebuild/test_unit_conv/data.*` Further: I commented out test for blocked data structures in `test_prebuild/run_all_tests.sh`; this test can no longer be run, since blocked data structures required the host model to define the horizontal dimension for blocked data as `horizontal_loop_extent`. This test and all code related to supporting blocked data structures in `ccpp_prebuild.py` will be removed in a follow-up PR (out of scope of this PR). User interface changes?: No Fixes NCAR#521 Testing: **all tests pass** tests **added**: test that invalid horizontal dimensions are detected correctly for host and scheme metadata tests **removed**: commented out blocked data tests for `ccpp_prebuild` unit tests: system tests: manual testing: --------- Co-authored-by: Test User <[email protected]>
## Summary Allow for sub components of a DDT to be passed into the Group caps from the Host cap . ## Description This PR adds recursive searching for DDTs when creating the call strings to the Group caps. This preserves the full reference in the call string. Snippet from `test_host_ccpp_cap.F90` before the change to ddt_library.py: `sfc_up_sw=phys_state%sfc_up_sw(col_start:col_end)` After, with full reference in call string: `sfc_up_sw=phys_state%fluxSW%sfc_up_sw(col_start:col_end)` User interface changes?: No Fixes: NCAR#639 This is built on NCAR#646 Testing: New testing added to pass subfields of DDT into scheme. --------- Co-authored-by: Steve Goldhaber <[email protected]> Co-authored-by: Dom Heinzeller <[email protected]>
Updates metadata_table.py parsing to lowercase the following fields: * local name * standard name * dimensions Context: I ran into an issue where dimension variable standard names were not being found if they contain >=1 upper case letter. To reproduce, modify any dimension in any of the capgen tests to be mixed case. The test run will fail with this message: `Could not find dimension...` User interface changes?: No --------- Co-authored-by: Courtney Peverley <[email protected]>
…t; remove known ddt case sensitivity (NCAR#662) Add error handling for integer dimension indices; remove case sensitivity for known DDTs **Description** - Updates to scripts/parse_tools/parse_checkers.py to append additional information to the error message when numerical indices are supplied for dimensions - In scripts/metadata_table.py, remove case sensitivity for known DDTs - Also fixes issue when a line is exactly equal to the max fortran line length and does not need to be continued. User interface changes?: No Closes NCAR#661 - numerical dimension indices error in capgen Closes NCAR#664 - long standard name bug Testing: test removed: n/a unit tests: all PASS, added two new doctests system tests: manual testing: --------- Co-authored-by: Steve Goldhaber <[email protected]>
Refactoring of testing infrastructure
Changes include:
- Refactored python tests to leverage `unittest` framework.
- Replaced shell tests with equivalent wrapper from Python.
- Reduces value duplication from fortran/shell/python files
to just fortran and python file.
- Adds re-useable CMake infrastructure for capgen, advection,
ddthost, and var_compatability tests.
- Adds cmake functions to wrap calls to ccpp_capgen.py and
ccpp_datafile.py.
- Enabling re-use of openmp and mpi configuration for both library
and tests.
- Removing PGI compiler options and adding ifx support.
- Replaces manually calling fortran tests through run_test with ctest
- Removed a few un-used files
- Adds missing `dependencies` and `--exclude-required` tests.
- Removed explicit optimization flag setting and relying on default
CMake behavior from `CMAKE_BUILD_TYPE`.
- Removed `-DDEBUG` flag as generated code requires a `--debug` flag to
insert debug logic explicitly.
- Updated test/unit-test docs to reflect current build/run process.
- Added docs documentation to demonstrate building of docs.
User interface changes?: No
Fixes: None
Resolves NCAR#514
Testing:
test removed: None
unit tests:
system tests:
manual testing:
---------
Co-authored-by: Dom Heinzeller <[email protected]>
Co-authored-by: Courtney Peverley <[email protected]>
…CAR#666) Allow flexible Fortran scheme argument ordering In checking compatibility between Fortran and metadata, schemes were required to have arguments in the same order. This PR removes that restriction. User interface changes?: No Fixes: NCAR#665 Testing: unit tests: Modified one test Fortran file to use a different argument ordering in the Fortran scheme. system tests: manual testing: manually ran unit tests Co-authored-by: Steve Goldhaber <[email protected]>
… feature/pull_in_dev_20250724
peverwhee
left a comment
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.
SIMA tests pass.
Also I'm going to add Fixes: Nothing to my resume.
|
Last update for today. There are at least two issues:
OLD = GOOD CODE NEW = BROKEN CODE Obviously, there are two issues: (a) the information on where to find this variable in the host model is no longer complete; and (b) instead of local names as known to the host model, we have standard names for the indices, but local names for the actual variable. It is not clear to me immediately what exactly triggered this change, and the size of the pull request doesn't make it easier. Maybe some of it is also related to the lowercasing? |
|
After much debugging, I got it to complete the call to ccpp_prebuild.py and to compile NEPTUNE. Just need to sort out a few remaining quirks with uppercase vs lowercase suite names. |
|
@climbfuji The ccpp_prebuild step is failing in the UFS when I switch to this branch: parse_source.CCPPError: Invalid horizontal dimension, 'horizontal_loop_extent', at CCPP_typedefs.meta:13 This is a new one for me. I can only find references to these error messages in the unit-testing code, which shouldn't(?) be triggered during the Cmake step. Right? (And if so, is this new? And why is it breaking?) |
These might be coming from the CMake changes but I'd be very concerned since I don't see CCPP_typedefs.meta in the framework repo and that string is only hardcoded in the unit-tests directory, not the regression tests. Unless there's something I missed (definitely possible since there was a lot going on!), the unit-tests directory shouldn't be included by CMake. The only other place that I can see after a cursory glance of the error handler is on line 138 of parse_source.py: So two things come to mind right away:
|
|
@dustinswales I think that's due to #659 I'm guessing those |
|
@peverwhee Is right, see https://github.com/NOAA-EMC/fv3atm/blob/e5b040cca39537499130260e0b65425543416f03/ccpp/data/CCPP_typedefs.meta On the UFS side, with #669 pulled into #668, you will need to make at least two other changes:
|
|
Thanks @peverwhee and @climbfuji! |
**Makes N m-2 equivalent to Pa.** Simple change to `unit_conversion.py` to add N m-2 equivalent to Pa and vice-versa. User interface changes?: No
…apgen parser (NCAR#669) **Bug fixes for ccpp_prebuild to work with partially case-insensitive capgen parser** These updates are needed to make `ccpp_prebuild.py` work with the recent, partially complete case-insensitive `capgen` parser. I tested this with NEPTUNE in a rather complicated way - pulling develop into the branch neptune uses (that is based on main), then creating the bug fixes there, then cherry-picking them so that we can merge them into develop here. Hopefully, by the time this comes all back to NEPTUNE it will still work :-) This PR needs to be merged into develop, then NCAR#668 must be updated before it can be merged into main. User interface changes?: no - but prebuild is now case-insensitive Fixes: no separate issue created, see discussion in NCAR#668 Testing: test removed: none unit tests: all pass system tests: all pass manual testing: full regression testing with NEPTUNE passed
… feature/pull_in_dev_20250724
|
@peverwhee @dustinswales @mkavulich @grantfirl I pulled develop into this branch on Aug 21 so that it now contains the bug fixes from #669. We should all retest with this PR before we merge this into develop (sorry @peverwhee if you have to run the tests twice). |
peverwhee
left a comment
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.
sima tests pass!
|
Just checking in, it looks like SIMA and NEPTUNE regtests have passed, we're just waiting on UFS now? |
|
@mkavulich This is correct. |
dustinswales
left a comment
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.
All UWM tests pass except single test with rrtmgp.
Bug in rrtmgp code will be addressed at a later time.
|
Background on the RRTMGP b4b failure in the UFS: We tracked this down to a bug in the memory handling on the UFS side. A variable used by RRTMGP (which is in the radiation group of the CCPP suite) is valid in the physics group of the suite only. The variable needs identified and then be moved from the interstitial DDT, that is non-persistent by definition, to a persistent DDT. This confirms that the |
|
@mkavulich Let's merge please if you are ok |
mkavulich
left a comment
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.
Adding a third approval since Dom's implicit approval as the PR opener doesn't count
All in the title.
Testing (cleared the board after merging #669 into develop and then into this branch/PR):
User interface changes?: No/Yes
Fixes: Nothing.
Testing: See above