Metadata corrections#474
Conversation
mcgibbon
requested review from
JulieSchramm,
climbfuji,
grantfirl and
llpcarson
as code owners
|
Closing as I've found some more errors from updating to the latest physics code, until those are fixed (and my auto-generated code compiles again). |
|
Fixed the remaining out-of-order arguments. |
climbfuji
left a comment
climbfuji
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR. There are quite a few places where you actually found inconsistencies in our metadata (correct intent, for example). Let me note, however, that for some metadata attributes, there are default values in case they are not specified, for example:
- default for optional is
F - default for intent is
in
I personally prefer to spell the attributes out, even if they match the default value, which is what your PR does.
I left a few comments with one change that needs to be reverted.
Thanks for fixing the order of arguments in the metadata files, in particular for RUC LSM.
| @@ -1,10 +1,10 @@ | |||
| [ccpp-arg-table] | |||
| name = gscond_init | |||
| name = zhaocarr_gscond_init | |||
There was a problem hiding this comment.
This [ccpp-arg-table] section should be removed altogether, there are no hooks in gscond.f that would trigger reading it.
There was a problem hiding this comment.
Is there a standard rule for this? I included these since in sfc_ocean (the module I was testing on), these init and finalize sections are included in the meta file despite the routines being empty.
There was a problem hiding this comment.
No, there isn't - at least in the current ccpp_prebuild.py version, because the parser goes through the Fortran scheme files that contain the CCPP entry points and only if it finds a metadata table hook (\section arg_table ... \htmlinclude) it will look for the corresponding entry in the .meta file. For the new cap_gen.py, this may be slightly different.
| long_name = minimum large ice fraction in F-A mp scheme | ||
| units = frac | ||
| dimensions = (2) | ||
| dimensions = (horizontal_dimension) |
There was a problem hiding this comment.
This is not correct. flgmin is - in FV3 - a control parameter that is independent of horizontal grid decomposition and has length 2. From GFS_typedefs.F90:
real(kind=kind_phys) :: flgmin(2) = (/0.180,0.220/) !< [in] ice fraction bounds
There was a problem hiding this comment.
Good catch, fixed.
| ######################################################################## | ||
| [ccpp-arg-table] | ||
| name = gscond_finalize | ||
| name = zhaocarr_gscond_finalize |
There was a problem hiding this comment.
This [ccpp-arg-table] section should be removed altogether, there are no hooks in gscond.f that would trigger reading it.
| long_name = surface pressure | ||
| units = Pa | ||
| dimensions = (horizontal_dimension) | ||
| dimensions = (horizontal_loop_extent) |
There was a problem hiding this comment.
Strictly speaking, all "horizontal_dimension" entries in any of the _run routines should use horizontal_loop_extent and not horizontal_dimension. Right now, we have a workaround in ccpp_prebuild.py that substitutes horizontal_loop_extent with horizontal_dimension for _init and _finalize routines, and vice versa for _run routines. It doesn't hurt at all making these changes here, but sfc_ocean.meta will be an outlier compared to all other schemes using (the incorrect) horizontal_dimension. But it would be better to not change it as part of this PR, so that we can make all adjustments of horizontal_loop_extent <-> horizontal_dimension in a single pull request.
There was a problem hiding this comment.
I will need these to be fixed in order for the wrapped routines to run, but I can fix the ones I'm testing on in a separate branch.
There was a problem hiding this comment.
Ok, thanks. We'll make this change everywhere soon.
capgen does not have a default for |
Thanks for correcting me - better trust your memory than mine. In any case, I prefer to spell out these attributes. |
3 participants
In using code generation to make Python wrappers for Fortran routines based on their
.metafiles, I have found some cases where dimension names are inaccurate or in the wrong order, or where intent and optional are missing as keys. In one case, a routine name was inaccurate, and in a couple cases an input was duplicated. I've also corrected the argument order for some (but not alledit: done) routines where they were out of order.Let me know if you'd like this merged to a different branch, or not merged at all.