Constituent updates #549

peverwhee · 2024-03-18T20:37:15Z

Summary

Brings in optional metadata field to enable dynamic (run-time) constituents; also brings in a couple new methods for the constituent object.

closes #557

Description

Key dynamic constituent mods:

ccpp_capgen.py: create dynamic constituent dictionary (key=scheme name) and pass into API()
constituents.py: add handling of dynamic constituents to register constituents routine (including conditional call(s) to user-supplied routines); update error handling for register_constituents

Includes additional constituent object methods:

set_water_species
set_min_val
set_molar_mass

Includes fixes to ensure consistent order of generated code (use statements, variables, etc)

Also brings back the .github/workflow/python.yaml workflow that got lost somehow

User interface changes?: Yes, but optional
User can now use "dynamic_constituent_routine" metadata header field to point to a function contained within the module fortran.

Testing:
Modified advection test to include dynamic constituent routines.
Added doctests to ccpp_datafile

Generated code snippet

   subroutine test_host_ccpp_register_constituents(host_constituents, errflg, errmsg)
      ! Create constituent object for suites in 

      use ccpp_constituent_prop_mod, only: ccpp_constituent_properties_t
      ! Suite constituent interfaces
      use ccpp_cld_suite_cap,        only: cld_suite_constituents_num_consts
      use ccpp_cld_suite_cap,        only: cld_suite_constituents_const_name
      use ccpp_cld_suite_cap,        only: cld_suite_constituents_copy_const
      ! Dynamic constituent routines
      use cld_liq, only: cld_liq_dynamic_constituents
      use cld_ice, only: cld_ice_dynamic_constituents

      ! Dummy arguments
      type(ccpp_constituent_properties_t), target, intent(in)  :: host_constituents(:)
      character(len=512), intent(out)   :: errmsg ! ccpp_error_message
      integer,            intent(out)   :: errflg ! ccpp_error_code
      ! Local variables
      integer                                      :: num_suite_consts
      integer                                      :: num_consts
      integer                                      :: num_dyn_consts
      integer                                      :: index, index_start
      integer                                      :: field_ind
      type(ccpp_constituent_properties_t), pointer :: const_prop
      ! dynamic constituent props variable for cld_ice
      type(ccpp_constituent_properties_t), allocatable :: dyn_const_prop_0(:)
      ! dynamic constituent props variable for cld_liq
      type(ccpp_constituent_properties_t), allocatable :: dyn_const_prop_1(:)

      errflg = 0
      num_consts = size(host_constituents, 1)
      num_dyn_consts = 0
      ! Number of suite constants for cld_suite
      num_suite_consts = cld_suite_constituents_num_consts(errflg=errflg, errmsg=errmsg)
      if (errflg /= 0) then
         return
      end if
      num_consts = num_consts + num_suite_consts
      ! Add in dynamic constituents
      call cld_ice_dynamic_constituents(dyn_const_prop_0, errcode=errflg, errmsg=errmsg)
      if (errflg /= 0) then
         return
      end if
      num_dyn_consts = num_dyn_consts + size(dyn_const_prop_0)
      call cld_liq_dynamic_constituents(dyn_const_prop_1, errcode=errflg, errmsg=errmsg)
      if (errflg /= 0) then
         return
      end if
      num_dyn_consts = num_dyn_consts + size(dyn_const_prop_1)
      num_consts = num_consts + num_dyn_consts
      ! Pack dynamic_constituents array
      allocate(dynamic_constituents(num_dyn_consts), stat=errflg)
      if (errflg /= 0) then
         errmsg = 'failed to allocate dynamic_constituents'
         return
      end if
      index_start = 0
      do index = 1, size(dyn_const_prop_0, 1)
         dynamic_constituents(index + index_start) = dyn_const_prop_0(index)
      end do
      index_start = size(dyn_const_prop_0, 1)
      deallocate(dyn_const_prop_0)
      do index = 1, size(dyn_const_prop_1, 1)
         dynamic_constituents(index + index_start) = dyn_const_prop_1(index)
      end do
      index_start = size(dyn_const_prop_1, 1)
      deallocate(dyn_const_prop_1)
      ! Initialize constituent data and field object
      call test_host_constituents_obj%initialize_table(num_consts)
      ! Add host model constituent metadata
      do index = 1, size(host_constituents, 1)
         const_prop => host_constituents(index)
         call test_host_constituents_obj%new_field(const_prop, errcode=errflg, errmsg=errmsg)
         nullify(const_prop)
         if (errflg /= 0) then
            return
         end if
      end do

      ! Add dynamic constituent properties
      do index = 1, size(dynamic_constituents, 1)
         const_prop => dynamic_constituents(index)
         call test_host_constituents_obj%new_field(const_prop, errcode=errflg, errmsg=errmsg)
         nullify(const_prop)
         if (errflg /= 0) then
            return
         end if
      end do
      ! Add cld_suite constituent metadata
      num_suite_consts = cld_suite_constituents_num_consts(errflg=errflg, errmsg=errmsg)
      if (errflg /= 0) then
         return
      end if
      do index = 1, num_suite_consts
         allocate(const_prop, stat=errflg)
         if (errflg /= 0) then
            errmsg = "ERROR allocating const_prop"
            return
         end if
         call cld_suite_constituents_copy_const(index, const_prop, errflg=errflg, errmsg=errmsg)
         if (errflg /= 0) then
            return
         end if
         call test_host_constituents_obj%new_field(const_prop, errcode=errflg, errmsg=errmsg)
         nullify(const_prop)
         if (errflg /= 0) then
            return
         end if
      end do

      call test_host_constituents_obj%lock_table(errcode=errflg, errmsg=errmsg)
      if (errflg /= 0) then
         return
      end if
! Set the index for each active constituent
      do index = 1, SIZE(test_host_model_const_indices)
         call test_host_constituents_obj%const_index(field_ind,                                   &
              test_host_model_const_stdnames(index), errcode=errflg, errmsg=errmsg)
         if (errflg /= 0) then
            return
         end if
         if (field_ind > 0) then
            test_host_model_const_indices(index) = field_ind
         else
            errflg = 1
            errmsg = 'No field index for '//trim(test_host_model_const_stdnames(index))
            return
         end if
      end do

Allow schema file to be passed to validate Add Fortran comment write method Add metadata table type and name to CCPP datatable variable entries Improve generated constituent-handling code Fix issues with output of long comments Ensure more instance variables are 'private' Have capgen create database object Add line fill (target line length) interface to FortranWriter New Fortran interface, indent_size New Fortran interface, blank_line Added insert function for verbatim copy to file Allow arbitrary break for long lines Make sure call list variables have an intent Added link to constituent dictionary Add metadata to CCPP constituent object DDT Add Fortran writing unit tests and fix line break bugs Added constituent props array host interface, improved unit conversion error Use +ELLIPSIS instead of +IGNORE_EXCEPTION_DETAILS for doctests Constituent cleanup, no function side effects in interface Completed and optimized constituent accessor routines Added minimum allowed value property

Add molecular weight as variable and constituent property Fix missing property (advected) from database

…ariable in hash_table

Rename dir for var_action test to distinguish from ct_build

…d variable arrays

…nstituents variable in cap rather than provided by host

mwaxmonsky

Looks great @peverwhee!

Has a few suggestions and some things I just wanted to clarify but just let me know if these are not relevant and I can re-review. Thanks!

scripts/host_cap.py

scripts/ccpp_capgen.py

scripts/ccpp_datafile.py

peverwhee · 2024-04-28T03:20:02Z

Some test suggestion:

Test for handling of duplicate compatible constituent

Test for handling of duplicate semi-compatible constituent (e.g., unit difference), that the framework cannot handle.

Test for handling of duplicate incompatible constituent

Added logic to the constituents object to check for compatibility before adding a new constituent. Right now, that logic only checks a logical and character properties (advected, water species, thermo active, units). And added the following tests to the advection_test:

Compatible constituent
incompatible constituent

Also added unit tests to test parsing and fortran vs metadata comparisons

climbfuji

One comment I have is that this PR is quite big and it contains a lot of somewhat unrelated changes. It would be better imo to separate out these different changes into individual PRs: f-strings, sorted, the changes to the water/minval/units, and the dynamic constituents?

I am not suggesting to break up this PR, but maybe consider submitting smaller, less intimidating to review changes in the future (in full awareness that I am guilty of these catchall PRs from time to time ...).

I am also not suggesting to make any changes to the style of the comments. This could be a separate PR where we clean up just the formatting and re-enable the linter(s) in CI.

Overall this looks very good to me, but I don't understand it well enough. I requested a couple of small changes and I will approve afterwards, but someone with a better understanding should be reviewing and approving this (and of course the full testing needs to be done once the reviews are finalized) before it gets merged.

Thanks for all the hard work that went into this PR!

pytest.ini

scripts/ccpp_datafile.py

src/ccpp_hash_table.F90

test/advection_test/test_host.F90

test/unit_tests/sample_scheme_files/duplicate_dyn_const.F90

climbfuji

Thanks for addressing my comments. This is a non-expert approval which on its own is not sufficient for merging ;-)

dustinswales

@peverwhee Some minor comments, but overall looks good. Thanks for these changes!
I will start testing in the UFS using this branch and push to get on the commit queue asap.

dustinswales · 2024-05-29T16:24:00Z

scripts/ccpp_capgen.py

@@ -629,7 +679,24 @@ def capgen(run_env, return_db=False):
        scheme_files = [const_prop_mod] + scheme_files
    # end if
    # Next, parse the scheme files
+    # We always need to parse the ccpp_constituent_prop_ptr_t DDT


What happens if we aren't using the constituents feature?

dustinswales · 2024-05-29T16:49:34Z

scripts/constituents.py

+        cap.write(f"integer{spc} :: num_consts", 2)
+        cap.write(f"integer{spc} :: num_dyn_consts", 2)
+        cap.write(f"integer{spc} :: index, index_start", 2)
+        cap.write(f"integer{spc} :: field_ind", 2)
        cap.write(f"type({CONST_PROP_TYPE}), pointer :: const_prop", 2)


Should this be cap.write(f"type({CONST_PROP_TYPE}), pointer :: const_prop => null()", 2)

dustinswales · 2024-05-29T16:58:38Z

src/ccpp_constituent_prop_mod.F90

-      character(len=*), parameter :: subname = 'ccp_model_const_add_metadata'
+      character(len=errmsg_len)                    :: error
+      character(len=*), parameter                  :: subname = 'ccp_model_const_add_metadata'
+      type(ccpp_constituent_properties_t), pointer :: cprop


Initialize to null()

dustinswales · 2024-05-29T17:04:29Z

src/ccpp_constituent_prop_mod.F90

-!!XXgoldyXX: Add check on key to see if incompatible item already there.
+         ! Check to see if standard name is already in the table
+         call field_data%standard_name(standard_name, errcode, errmsg)
+         cprop => this%find_const(standard_name)


I see that you nullify cprop at the start of this%find_const().
Is it best to pass in a null pointer, and return the null pointer if condition not met, otherwise return associated pointer. A purely stylistic question.

dustinswales · 2024-05-29T17:08:46Z

src/ccpp_constituent_prop_mod.F90

+         cprop => this%find_const(standard_name)
+         if (associated(cprop)) then
+            ! Standard name already in table, let's see if the existing constituent is the same
+            cindex = cprop%const_index()


dustinswales · 2024-05-29T17:18:37Z

test/advection_test/test_host.F90

+      call test_host_ccpp_deallocate_dynamic_constituents()
+      deallocate(host_constituents)
+      allocate(host_constituents(2))
+      call host_constituents(1)%instantiate(std_name="specific_humidity",     &


I probably don't understand the test here, but how is it allowed to have two host constituents with the same standard_name?

dustinswales · 2024-05-29T17:23:41Z

test/advection_test/test_host.F90

+         errflg = 0
+      end if
+      ! Check moist mixing ratio for a dynamic constituent
+      call const_props(index_dyn2)%is_dry(const_log, errflg, errmsg)


Should this be calling "is_moist"?

dustinswales · 2024-05-29T17:34:53Z

test/unit_tests/sample_scheme_files/duplicate_dyn_const.F90

@@ -0,0 +1,96 @@
+! Test parameterization with no vertical level


Can you update the description?
Also, is this test exercising duplicates?

dustinswales · 2024-05-29T17:35:01Z

test/unit_tests/sample_scheme_files/dyn_const_not_present.F90

@@ -0,0 +1,75 @@
+! Test parameterization with no vertical level


Can you update the description?

dustinswales · 2024-05-29T17:36:59Z

test/var_compatibility_test/run_test

@@ -6,7 +6,7 @@ scriptdir="$( cd $( dirname $0 ); pwd -P )"
 ##
 ## Option default values
 ##
-defdir="ct_build"
+defdir="va_build"


Is this needed?

Steve Goldhaber and others added 30 commits July 13, 2022 21:45

Avoid RE deprecation warning by moving case fold to state_machine.py

bce4d22

Fix doctest

279c180

Allow '1' to be a valid unit

057588e

Added molar_mass (molecular weight) value to constituent property type

678621e

initial changes

27f6851

fixes

ca65368

address review comments

10416a9

hopefully final fixes

5e8e33a

Merge pull request NCAR#41 from peverwhee/add_molecular_weight

9bb155b

Add molecular weight as variable and constituent property Fix missing property (advected) from database

initial commit

a2386d6

fix function name

7657f33

fix fstring

5315500

fix mistakes in routine names and comments

84bade6

split up register and initialize

3494311

cleanup

d9871c0

cleanup

a71e1b5

add temporary hash_table variable to avoid dangling pointer

6c7c82a

deallocate hash_table

d4ad4aa

cleaner way to deallocate hash table in iterator object

f549d64

change name of const array interfaces

5813d19

add intialized_in_physics property

8ff8d5a

better error handling; fix for copy constituent

f074f1c

remove unnecessary use statment

5e839bf

fix unit tests

feb8bcb

revert changes for unit conversion test

15f52c6

handle constituent edge cases better and differently; revert module v…

3703645

…ariable in hash_table

fix decimal

1e83c4c

Merged changes from new_variables_input

31919df

Rename dir for var_action test to distinguish from ct_build

remove initialized-in-physics constituent from advection test require…

dad0dbd

…d variable arrays

Courtney Peverley and others added 4 commits April 24, 2024 01:42

add check that routine exists in fortran; use module-level dynamic co…

adff339

…nstituents variable in cap rather than provided by host

fix typo

d29a035

code cleanup; code review

9ce41ee

allow for skipping of ddt check; skip ddt check from offline script

eee30cb

mwaxmonsky requested changes Apr 25, 2024

View reviewed changes

Courtney Peverley added 3 commits April 26, 2024 16:24

handle dimensions for promoted variables

807d35a

check for compatibility before adding constituent; add more testing

da55869

add unit comparison

2a23124

code review

4c4807b

peverwhee requested a review from mwaxmonsky April 28, 2024 05:26

update logic to exclude nested subroutines from routine parsing

12b4eab

peverwhee requested a review from gold2718 April 29, 2024 19:43

update vardictionary doctests and add 3.12 testing

3a6a3e2

climbfuji requested changes May 3, 2024

View reviewed changes

Courtney Peverley and others added 6 commits May 6, 2024 16:26

Merge remote-tracking branch 'NCAR/main' into group-fix

8c90217

code cleanup; review requests

9e43b78

code clean-up; add test for promoted variable with no horiz dimension

b668b2e

code review; cleanup

1e139b5

Merge remote-tracking branch 'NCAR/main' into add_const_interface

522ad25

only run pytests on test/ directory in python.yaml workflow

15a16ea

peverwhee requested a review from climbfuji May 16, 2024 21:17

climbfuji approved these changes May 17, 2024

View reviewed changes

Courtney Peverley added 2 commits May 20, 2024 15:30

merge in optional fortran v metadata checker

c06600e

Merge remote-tracking branch 'origin/group-fix' into add_const_interface

0b8f23a

This was referenced May 20, 2024

Add script to check fortran vs metadata without a host model #558

Closed

Handle dimensions for promoted variables #562

Closed

mattldawson mentioned this pull request May 28, 2024

Register species from MICM ESCOMP/atmospheric_physics#93

Open

Merge remote-tracking branch 'NCAR/main' into add_const_interface

c781edc

dustinswales requested changes May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constituent updates #549

Constituent updates #549

peverwhee commented Mar 18, 2024 •

edited

mwaxmonsky left a comment

peverwhee commented Apr 28, 2024

climbfuji left a comment

climbfuji left a comment

dustinswales left a comment

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

dustinswales May 29, 2024

		@@ -0,0 +1,96 @@
		! Test parameterization with no vertical level

		@@ -0,0 +1,75 @@
		! Test parameterization with no vertical level

Constituent updates #549

Are you sure you want to change the base?

Constituent updates #549

Conversation

peverwhee commented Mar 18, 2024 • edited

Summary

Description

Generated code snippet

mwaxmonsky left a comment

Choose a reason for hiding this comment

peverwhee commented Apr 28, 2024

climbfuji left a comment

Choose a reason for hiding this comment

climbfuji left a comment

Choose a reason for hiding this comment

dustinswales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peverwhee commented Mar 18, 2024 •

edited