master: update from gmtb develop 2019/09/09 by climbfuji · Pull Request #222 · NCAR/ccpp-framework

climbfuji · 2019-09-09T03:23:19Z

This PR updates the ccpp-framework master branch with the cumulative changes from gmtb/develop.

These changes are

updates of the CCPP prebuild Python scripts to work with the new and the old metadata format at the same time
conversion of ccpp_types.F90 to the new metadata format
updates to the technical documentation (to be moved to a separate repository soon)

All of these changes have been approved previously by the GMTB code owners @grantfirl and @llpcarson and mostly also by @gold2718.

…se, skip optional and intent attributes for variable/type definitions

…ormat and convert back into old format, currently tested for schemes and module variable definitions, not yet for DDTs

…ct answer when checking logicals

…ype and kind definitions when initializing from a metadata table

…standard names

…riable names that denote the start of a new variable block

…y: define and export LITERAL, use in logical expressions for array references

…py: add capability to parse variable and type definition metadata tables in new metadata format on a per-file basis

…uard to prevent using multi-dimensional character arrays

…red, this only works if all DDTs are defined in the same file and in the correct order

…tments to convert type/variable definitions and fill in dimensions, array references, ...

…e tables one by one, filling in dimension information from type definitions

…code

…into new_metadata_format_step1_20190725

…20190725 Migrate to new metadata format step 1

…, not for type definitions/module variables

…pment work for generating html table from metadata table

…version of metadata-to-html converter

… to specified output directory

…nk metadata tables

…if old metadata is found

TechDoc changes for new metadata format (target gmtb/develop)

…ger needed since we can parse the separate .meta files directly

…_20190904 Convert all schemes to new metadata, new unit conversion micrometer<->meter, merge gsd/develop

…into HEAD

codecov-io · 2019-09-09T03:24:55Z

Codecov Report

Merging #222 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #222   +/-   ##
=======================================
  Coverage   47.28%   47.28%           
=======================================
  Files          14       14           
  Lines        1343     1343           
=======================================
  Hits          635      635           
  Misses        708      708

Impacted Files	Coverage Δ
src/ccpp_types.F90	`75% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbec0e5...7b0ed0b. Read the comment docs.

climbfuji · 2019-09-09T03:24:56Z

Associated PRs (GitHub) or changes (Vlab/gerrit):

MISSING

climbfuji · 2019-09-09T03:37:24Z

Associated pull requests (GitHub) or code review changes (Vlab/Gerrit):

NEMSfv3gfs: https://vlab.ncep.noaa.gov/code-review/19568 (including update of submodule pointer for WW3)
FV3: https://vlab.ncep.noaa.gov/code-review/19567
FV3/atmos_cubed_sphere: NOAA-EMC/GFDL_atmos_cubed_sphere#3
NEMS: NOAA-EMC/NEMS#7 (ignore update of submodule pointer for pyprodutil)
WW3: no changes
FMS: no changes
ccpp-physics NCAR/ccpp-physics#317
ccpp-framework #222

grantfirl

Approved to master since already reviewed/approved in gmtb/develop branch.

gold2718

Some questions, some concerns about NOAA-specific documentation, and some small change requests.

gold2718 · 2019-09-13T14:35:16Z

-    * If the variables are already available, they can be invoked in the scheme’s metadata table and one can skip the rest of this subsection. If the variable required is not available, consider if it can be calculated from the existing variables in the CCPP. If so, an interstitial scheme (such as ``scheme_pre``; see more in :numref:`Chapter %s <CompliantPhysParams>`) can be created to calculate the variable. However, the variable must be defined but not initialized in the host model as the memory for this variable must be allocated on the host model side.  Instructions for how to add variables to the host model side is described in :numref:`Chapter %s <Host-side Coding>`.
+    * If the variables are already available, they can be invoked in the scheme’s metadata file and one can skip the rest of this subsection. If the variable required is not available, consider if it can be calculated from the existing variables in the CCPP. If so, an interstitial scheme (such as ``scheme_pre``; see more in :numref:`Chapter %s <CompliantPhysParams>`) can be created to calculate the variable. However, the variable must be defined but not initialized in the host model as the memory for this variable must be allocated on the host model side.  Instructions for how to add variables to the host model side is described in :numref:`Chapter %s <Host-side Coding>`.
+
+    * If new namelist variables need to be added, the ``GFS_control_type`` DDT should be used. In this case, it is also important to modify the namelist file ``input.nml`` to include the new variable.


This seems rather GFS specific. Is this really a framework item?

We were discussing to move the technical documentation outside of ccpp-framework, or at least parts of it. Let's keep those changes for the moment and clean the documentation up when the move is made. Ok?

Sure, let's keep an eye on this issue moving forwards.

gold2718 · 2019-09-13T15:06:12Z

+
+* ``<type>`` can be ``scheme``, ``module``, ``DDT``, or ``host``.
+
+* For empty schemes, the three lines above are sufficient. For non-empty schemes, the metadata should


should or must?

must - will change

gold2718 · 2019-09-13T15:35:04Z

 ==============================

-The :term:`SDF` is a file in XML format used to specify the name of the suite, the physics schemes to run, groups of physics that run together, the order in which to run the physics, and whether subcycling will be used to run any of the parameterizations with shorter timesteps. The :term:`SDF` files are located in ``ccpp/suites/suite_*.xml``. 
+The :term:`SDF` is a file in XML format used to specify the name of the suite, the physics schemes to run, groups of physics that run together, the order in which to run the physics, and whether subcycling will be used to run any of the parameterizations with shorter timesteps. The :term:`SDF` files are located in ``ccpp/suites/suite_*.xml``.


The location seems model specific. Do the suites have to be in any special place for ccpp_prebuild?

Are you OK with "The :term:SDF files are part of the host model code"?

We plan to store them in our physics library.
For now, your suggestion might be okay because we do not have the ability to write host-model portable suites. Make your suggested change and let's keep an eye on it.

gold2718 · 2019-09-13T15:51:42Z

+    if not len(items) in [1, 2, 3]:
+        raise Exception("decode_container not implemented for {0} items".format(len(items)))
+    itemsdict = {}
+    for i in xrange(len(items)):


Side note: xrange does not exist in python 3 so consider not using it anymore,

Noted. The CCPP prebuild scripts are only compatible with Python 2.7.x - something that has been criticized rightfully by others. Because we will move to capgen in the near (?) future, I am not making any attempts to make the CCPP prebuild scripts Python-3 compatible at this time.

Right. Note that capgen is not yet python 3 compatible (but I keep picking at it when I have time).

gold2718 · 2019-09-13T17:10:03Z

        elif self.type is bool:
            if isinstance(test_value, str):
-                valid_val = (test_value in ['True', 'False']) or (test_value.lower() in ['t', 'f', '.true.', '.false.'])
+                if test_value.lower() in VariableProperty.__true_vals + VariableProperty.__false_vals:


I thought we had decided that allowed logical literals were Fortran or Python allowed values. It seems you have expanded the set. Did I miss something?
Also, this list addition is done many, many times, would a combo list (e.g., __logical_vals) be better?

I actually made this change in "conjunction" (or at least per change request) from you. Let's keep it this way and improve in the future.

gold2718 · 2019-09-13T17:11:52Z

-                valid_val = (test_value in ['True', 'False']) or (test_value.lower() in ['t', 'f', '.true.', '.false.'])
+                if test_value.lower() in VariableProperty.__true_vals + VariableProperty.__false_vals:
+                    valid_val = (test_value.lower() in VariableProperty.__true_vals) or \
+                                not (test_value.lower() in VariableProperty.__false_vals)


Does the second clause of the or operator ever change the value given its position inside the if statement above? Why not just omit it?

Agreed, will change it

gold2718 · 2019-09-13T17:15:18Z

+                                     # are defined in that file and/or parsed beforehand.
+                                     #check_fn_in=check_fortran_type),
+                                     # *DH
+                                     ),


What about other types? This represents a big change in how types are checked.
If you really need to disable checks on DDTs, please do it in check_fortran_type, not here. Also, let me know why this is necessary.

We discussed this either in the original PR that introduced the change or in the ccpp-framework meeting. The current version of the parsing scripts for the new metadata rely on all DDTs referenced in one metadata table to be declared in the same metadata table (and also the order matters, they have to be declared beforehand). This doesn't work when radiation sub-DDTs are declared elsewhere and referenced in GFS_typedefs.meta. There won't be a simple fix for this, because we are only reading one (new) metadata file at a time with ccpp_prebuild.py.

I am also wondering if making the following change in check_fortran_type makes any difference in terms of how big the change is:

Turn

dt = "" match = check_fortran_intrinsic(typestr, error=False) if match is None: match = registered_fortran_ddt_name(typestr) dt = " derived" # End if if match is None: if error: raise CCPPError("'{}' is not a valid{} Fortran type".format(typestr, dt)) else: typestr = None # End if # End if return typestr

to

dt = "" match = check_fortran_intrinsic(typestr, error=False) if match is None: match = registered_fortran_ddt_name(typestr) dt = " derived" # End if #if match is None: # if error: # raise CCPPError("'{}' is not a valid{} Fortran type".format(typestr, dt)) # else: # typestr = None # # End if # End if return typestr

?

That is what I was thinking, at least for now. We should work towards a more robust solution that takes various host build systems into account.

gold2718 · 2019-09-13T17:20:46Z

 from parse_source   import CCPPError, context_string
 from parse_object   import ParseObject
-from parse_checkers import check_fortran_id, FORTRAN_ID
+from parse_checkers import check_fortran_id, LITERAL, FORTRAN_ID


I do not really like this name (LITERAL) because it only covers integers. Can it be changed to LITERAL_INT or something similar?

Will use LITERAL_INT

Thanks, sorry for being picky.

gold2718 · 2019-09-13T17:22:08Z


 ########################################################################

+# LITERAL is a strin representing a literal value


string, not strin, also, the comment is .false. as it only represents integer literal values.

I will correct this

climbfuji · 2019-09-13T17:40:05Z

We are in a bit of a difficult situation here. While I agree with @gold2718's comments, we had all PRs going into gmtb/develop approved by not only GMTB but also by @gold2718 (maybe we didn't communicate properly that we interpreted "good enough for gmtb/develop" as "good enough for master"). The changes in this PR have been used for generating the new NEMSfv3gfs baselines and I am not comfortable making changes here when the regression testing is near completion.

There seem to be some hierarchy involved here that we need to obey, so maybe from the next time we have the changes for CCPP/master approved (and tested) before we propose changes to EMC.

What do we do? Is it ok to record @gold2718's comments and corrections and include them in the following commit for EMC (scheduled for 10/04)? Or do we need to tell EMC that we are making updates and that they have to hold and restart their regression tests?

climbfuji · 2019-09-13T17:52:57Z

Note that this issue will be temporary (at least I hope so), because EMC will soon create its own institutional fork of ccpp-{framework,physics} master. This will separate the authoritative repository from their testing.

ligiabernardet · 2019-09-13T19:17:12Z

Given that we are targeting an update of EMC and this Monday, and missing this date would set us back significantly, I favor going ahead with this commit and introducing the updates in the next commit cycle.

…

On Fri, Sep 13, 2019 at 1:13 PM Dom Heinzeller ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/CCPPtechnical/source/CompliantPhysicsParams.rst <#222 (comment)>: > + +* For each CCPP compliant scheme, the ``.meta`` file should have this set of lines + +.. code-block:: fortran + + [ccpp-arg-table] + name = <name> + type = <type> + +* ``ccpp-arg-table`` indicates the start of a new metadata section for a given scheme. + +* ``<name>`` is name of the corresponding subroutine/module. + +* ``<type>`` can be ``scheme``, ``module``, ``DDT``, or ``host``. + +* For empty schemes, the three lines above are sufficient. For non-empty schemes, the metadata should must - will change — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#222?email_source=notifications&email_token=AE7WQASWRFW4RMT6OM7D4HDQJPQ6BA5CNFSM4IUWEDK2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEW2F6I#discussion_r324332004>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE7WQARTDZ7JH2EPYAAQOOTQJPQ6BANCNFSM4IUWEDKQ> .

gold2718 · 2019-09-13T20:06:03Z

So, except for the typos, make the changes in a gmtb/develop commit as soon as practical?

climbfuji · 2019-09-13T20:08:01Z

So, except for the typos, make the changes in a gmtb/develop commit as soon as practical?

I have given EMC the hash of my latest commit 7b0ed0b that addresses some of your questions already. We have another commit scheduled for master on 10/04 in which we could bring the changes from gmtb/develop in.

climbfuji · 2019-09-13T20:46:52Z

Thanks for approving. Will merge now.

gold2718 · 2019-09-13T20:46:55Z

Going ahead and approving then since I am going to bed :)

climbfuji · 2019-09-13T20:59:53Z

Going ahead and approving then since I am going to bed :)

Wish I could do the same.

gold2718 · 2019-09-14T19:42:37Z

Going ahead and approving then since I am going to bed :)

Wish I could do the same.

Well, I am 8 hours ahead so I had an excuse :)

climbfuji and others added 30 commits July 11, 2019 08:51

Remove Steve Goldhaber from CODEOWNERS for gmtb/develop branch

40567df

Update CODEOWNERS to GSD team

e0dff34

Update CODEOWNERS to enable branch protection

da96d72

scripts/convert_metadata.py: don't convert standard names to lower ca…

d7824f9

…se, skip optional and intent attributes for variable/type definitions

scripts/metadata_parser.py: add functionality to parse new metadata f…

9bf55e6

…ormat and convert back into old format, currently tested for schemes and module variable definitions, not yet for DDTs

scripts/metavar.py: bugfix in routine valid_value to return the corre…

3b531f0

…ct answer when checking logicals

scripts/mkcap.py: bugfix, perform type_kind_test correctly for both t…

c8423d2

…ype and kind definitions when initializing from a metadata table

scripts/parse_tools/parse_checkers.py: allow uppercase characters in …

3216854

…standard names

scripts/metadata_table.py: make script executable

3157123

scripts/metadata_table.py: adjust indents, allow literals in local va…

dc1cbca

…riable names that denote the start of a new variable block

scripts/metavar.py: use __true_vals and __false_vals

0dd8c35

scripts/parse_tools/__init__.py, scripts/parse_tools/parse_checkers.p…

8c8ac84

…y: define and export LITERAL, use in logical expressions for array references

scripts/ccpp_prebuild.py, scripts/common.py, scripts/metadata_parser.…

9bd830f

…py: add capability to parse variable and type definition metadata tables in new metadata format on a per-file basis

scripts/mkcap.py: remove unnecessary type_kind_var logic and legacy g…

9eac6a1

…uard to prevent using multi-dimensional character arrays

scripts/mkstatic.py: remove unnecessary import statement

a640f0a

scripts/metavar.py: do not check for derived data types being registe…

2bda79b

…red, this only works if all DDTs are defined in the same file and in the correct order

scripts/convert_metadata.py: add logic and host-model dependent adjus…

3ad6131

…tments to convert type/variable definitions and fill in dimensions, array references, ...

Allow comments in type definition lines

4d72408

scripts/convert_metadata_schemes_using_typedef_dims.py: convert schem…

4b61112

…e tables one by one, filling in dimension information from type definitions

Allow comments in type definition initial lines

3b1f982

scripts/mkcap.py: bugfix, remove remainder of previous type_kind_var …

39fe3bf

…code

Revert accidental import of new parse_checkers

4476a8b

Merge branch 'fix_ftype' of https://github.com/gold2718/ccpp-framework …

1e4240a

…into new_metadata_format_step1_20190725

Revert unintended whitespaces at the beginning of file metadata_table.py

aad2925

Merge pull request NCAR#205 from climbfuji/new_metadata_format_step1_…

8734529

…20190725 Migrate to new metadata format step 1

scripts/metavar.py: intent and optional are only for scheme variables…

9830424

…, not for type definitions/module variables

scripts/metadata_table.py, scripts/metadata2html.py: recording develo…

6c26eae

…pment work for generating html table from metadata table

scripts/metadata_table.py, scripts/metadata2html.py: initial working …

b3c96ec

…version of metadata-to-html converter

scripts/metadata_table.py, scripts/metadata2html.py: write html files…

eec1321

… to specified output directory

scripts/convert_metadata*.py: don't add htmlinclude statement for bla…

577ffaf

…nk metadata tables

climbfuji added 5 commits September 4, 2019 10:34

scripts/metadata_parser.py: warn or raise exception (currently warn) …

ca4991e

…if old metadata is found

Merge pull request NCAR#214 from ligiabernardet/TechDoc_updates_metadata

dfa7c0b

TechDoc changes for new metadata format (target gmtb/develop)

scripts/metadata_parser.py: remove legacy debug output to xml, no lon…

c1cf90f

…ger needed since we can parse the separate .meta files directly

Merge pull request NCAR#221 from climbfuji/gmtb_develop_monster_merge…

2d95f96

…_20190904 Convert all schemes to new metadata, new unit conversion micrometer<->meter, merge gsd/develop

Merge branch 'gmtb/develop' of https://github.com/NCAR/ccpp-framework …

6d1a383

…into HEAD

climbfuji mentioned this pull request Sep 9, 2019

master: update from gmtb develop 2019/09/09 NCAR/ccpp-physics#317

Merged

This was referenced Sep 9, 2019

This commit #refs 66173 (merge gmtb/develop into NOAA-EMC/dev/emc) NOAA-EMC/GFDL_atmos_cubed_sphere#3

Closed

This commit #refs 67538 (merge gmtb/develop into NOAA-EMC/develop) NOAA-EMC/NEMS#7

Merged

climbfuji marked this pull request as ready for review September 9, 2019 03:37

climbfuji requested review from gold2718, grantfirl and llpcarson as code owners September 9, 2019 03:37

grantfirl approved these changes Sep 13, 2019

View reviewed changes

gold2718 requested changes Sep 13, 2019

View reviewed changes

Update of scripts and documentation to address reviewer comments

7b0ed0b

gold2718 approved these changes Sep 13, 2019

View reviewed changes

climbfuji merged commit 257252e into NCAR:master Sep 13, 2019

climbfuji deleted the master_update_from_gmtb_develop_20190909 branch June 27, 2022 02:54


		* ``<type>`` can be ``scheme``, ``module``, ``DDT``, or ``host``.

		* For empty schemes, the three lines above are sufficient. For non-empty schemes, the metadata should


		########################################################################

		# LITERAL is a strin representing a literal value

Conversation

climbfuji commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

climbfuji commented Sep 9, 2019

Uh oh!

climbfuji commented Sep 9, 2019

Uh oh!

grantfirl left a comment

Choose a reason for hiding this comment

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

climbfuji commented Sep 13, 2019

Uh oh!

climbfuji commented Sep 13, 2019

Uh oh!

ligiabernardet commented Sep 13, 2019 via email

Uh oh!

gold2718 commented Sep 13, 2019

Uh oh!

climbfuji commented Sep 13, 2019

Uh oh!

climbfuji commented Sep 13, 2019

Uh oh!

gold2718 commented Sep 13, 2019

Uh oh!

climbfuji commented Sep 13, 2019

Uh oh!

climbfuji commented Sep 9, 2019 •

edited

Loading

codecov-io commented Sep 9, 2019 •

edited

Loading