Constituents by peverwhee · Pull Request #495 · NCAR/ccpp-framework

peverwhee · 2023-09-01T16:56:32Z

Summary

Updates to bring in constituents object and constituent properties object.

Description

Meat of the updates are in src/ccpp_constituent_prop_mod.F90
Add interfaces to host cap to access constituent and const props objects
Add metadata properties specific to constituents (currently advected=True is the only way to note a constituent
Adds ddt object, support, and testing to achieve above

User interface changes?: Yes
Can now optionally add constituent via metadata property advected=True

Addresses #282

Testing:

Ran with CAM-SIMA (SE dycore and KESSLER physics)
All tests pass except unit conversion test

dustinswales

@peverwhee @gold2718 Looks fine to me. Thanks for this monster effort. I don't have anything that would hold up this PR from being merged, assuming the new tests all work as expected.

I'm a bit confused about the need for metadata for fields within the DDTs, but maybe this is because some DDTs have column data that may need to be referenced outside of their home DDT?

dustinswales · 2023-10-03T16:35:28Z

@peverwhee Can you update this branch with updates from feature/capgen?

gold2718

Yowza, that is a lot to digest!
I have some question and a few suggested changes but overall, I think this is good.

mkavulich

I'll add these as comments since I don't understand enough of the big picture to call this a proper "review".

mkavulich · 2023-09-18T20:22:42Z

                                        module=spec_name,
                                        var_dict=var_dict)
                mheaders.append(mheader)
-                if run_env.logger and run_env.logger.isEnabledFor(logging.DEBUG):


This might be outside the scope of this PR specifically, but It seems like this same debug logic is used a lot in capgen, does it make sense to pull this out into a function so it can be a single line every time?

Added new function debug_on() routine to CCPPFrameworkEnv

mkavulich · 2023-09-18T20:25:49Z

                    ctx = context_string(pobj, nodir=True)
                    msg = 'Adding header {}{}'
                    run_env.logger.debug(msg.format(mheader.table_name, ctx))
+                # end if


I understand the need for helper comments like this for very long or convoluted logic blocks, but here (and other if blocks that are only a few lines) it seems superfluous.

I see that this is already present in many places so probably doesn't need to be addressed in this PR, but I thought I would throw it out there.

I would rather have consistent usage and have been saved more than once by these statements showing me where some code block was incorrectly indented.
This particular choice is part of the style guide of some projects I work on. Others use a blank line at the end of every block. I do not know any other project that has different rules for different sized blocks.

My personal preference would be blank lines, but since this project has started out with # end if (lowercase nowadays, for consistency) it makes sense to continue using it. I shall add those missing statements in my PR #512 later!

mkavulich · 2023-09-18T20:43:05Z

+            init_args = [f'std_name="{std_name}"', f'long_name="{long_name}"',
+                         f'units="{units}"', f'vertical_dim="{vertical_dim}"',
+                         f'advected={advect_str}',
+                         f'errcode={errvar_names["ccpp_error_code"]}',
+                         f'errmsg={errvar_names["ccpp_error_message"]}']


I'm curious what our new minimum python version is? If we require Python 3.8 or newer then this and similar expressions can be greatly simplified

Suggested change

init_args = [f'std_name="{std_name}"', f'long_name="{long_name}"',

f'units="{units}"', f'vertical_dim="{vertical_dim}"',

f'advected={advect_str}',

f'errcode={errvar_names["ccpp_error_code"]}',

f'errmsg={errvar_names["ccpp_error_message"]}']

init_args = [f'{std_name=}', f'{long_name=}',

f'{units=}', f'{vertical_dim=}',

f'{advect_str=}',

f'errcode={errvar_names["ccpp_error_code"]}',

f'errmsg={errvar_names["ccpp_error_message"]}']

Well that is a very interesting feature I had not noticed creeping in. Very Fortran friendly :)

BTW, I think line 315 has to stay as is. It is a Python string but a Fortran literal.

changed to new fancy python 3.8 version!

mkavulich · 2023-10-16T19:49:13Z

+               astat = 1
+               call set_errvars(astat, subname,                               &
+                    errcode=errcode, errmsg=errmsg,                           &
+                    errmsg2=" ERROR: num_advected_vars index out of bounds")


Can this error message also print num_vars and the current index?

mkavulich · 2023-10-16T19:50:21Z

+                     if (check) then
+                        index_advect = index_advect + 1
+                        if (index_advect > this%num_advected_vars) then
+                           call set_errvars(1, subname,                       &


Same here in these two blocks, print the OOB index and the limit it has exceeded

mwaxmonsky

Great work @peverwhee!! Just wanted to clarify a few things.

mwaxmonsky · 2023-10-23T16:40:42Z

+            else:
+                element_names = None


Is this else needed since we don't enter the if block that assigns element_names=[]?

agreed. removed.

But if element_names is not given a value, what is returned at the end?

Good point. At the end of the function if element_names is not defined, python throws an exception.

Would a better suggestion have been to set element_names = None by default before starting the if checks?

I don't know if it is "better" but it is sufficient. The nice thing about raising exceptions in logic corners that should never be hit is that you can find out if future coding ever takes this routine out of its design range.

thanks for this discussion, guys. I thoroughly confused myself and will blame the recursion (it definitely isn't a me problem!). Updated to initialize element_names to None at the top of the routine and then also raise an exception if we get to the else of if ddt_lib and (dtitle in ddt_lib):

gold2718 · 2023-11-01T09:58:20Z

+            else:
+                element_names = None


But if element_names is not given a value, what is returned at the end?

gold2718

Finished a review of ac18c44 and resolved all the conversations I started. However, I still see a few things I think should be changed.

gold2718 · 2023-11-01T10:12:17Z

+
+      if (associated(this%prop)) then
+         call this%prop%vertical_dimension(dimname)
+         is_2d = len_trim(dimname) == 0


For CAM at least, 2-D means there is no vertical dimension so there is no name for it. It might be cleaner and more understandable if a is_2d method was added to the prop obect.

climbfuji

This is a very high-level review. I was not looking at every single of the 10000-ish lines in the modified/added constituent code, but rather trying to get a bit of an understanding and look at things I know about (and care about more ...).

I am a little concerned about how complicated and extensive this code is, but as long as none of this is mandatory and host models can use capgen without the constituents logic (not saying the should, but at least for transitioning from prebuild to capgen that would be good) then that's all ok.

My only real concern is the newly added debug_on feature, which is inconsistent with how the arguments to capgen and prebuild are called and therefore should be renamed to just verbose. I started suggesting those changes until I got tired, and I certainly overlooked a few.

climbfuji · 2023-11-09T03:21:11Z

        CCPPFrameworkEnv object."""
        return self.__kind_dict.keys()

+    def debug_on(self):


This should not be debug_on but simply verbose to match the flag that gets passed to capgen, and also to avoid confusion with what the --debug switch for capgen is doing (see PR #512)

good call! Changed to a verbose property

climbfuji · 2023-11-09T03:21:41Z

            # end if
            lmsg = "Group {}, schemes = {}"
-            if run_env.logger and run_env.logger.isEnabledFor(logging.DEBUG):
+            if run_env.debug_on():


See below, should be just run_env.verbose() or even better a property run_env.verbose. This way it's consistent with #512 where I added run_env.debug.

climbfuji · 2023-11-09T03:21:46Z

        # Set name of module and filename of cap
        filename = '{module_name}.F90'.format(module_name=self.module)
-        if run_env.logger and run_env.logger.isEnabledFor(logging.DEBUG):
+        if run_env.debug_on():


climbfuji · 2023-11-09T03:27:51Z

-        cap.write("", 0)
-        cap.write("! Dummy arguments", 2)
+        cap.comment("Update constituent field info from <const_array>", 2)
+        cap.blank_line()


These (new?) additions blank_line and comment are useful!

climbfuji · 2023-11-09T03:28:28Z

+        # end if
        if ((not recur) and
-            run_env.logger and run_env.logger.isEnabledFor(logging.DEBUG)):
+            run_env.debug_on()):


Suggested change

run_env.debug_on()):

run_env.verbose()):

climbfuji · 2023-11-09T03:44:23Z

        self._local_dim_name = None
        super().__init__(index_name, context, parent, run_env)
-        if run_env.logger and run_env.logger.isEnabledFor(logging.DEBUG):
+        if run_env.debug_on():


Suggested change

if run_env.debug_on():

if run_env.verbose():

climbfuji · 2023-11-09T03:48:04Z

      end if

-   end function ccp_is_initialized
+   end function ccp_is_instantiated


I have a feeling that I asked this question 2 +/- years ago when the constituents where added originally. What does ccp and ccpt stand for? Should we be concerned about confusion with ccpp that also shows up as acronym in the same part of the code? (This is just for my curiosity or for the future, not really related to this PR)

Sorry, this is just me trying to save space with recursive acronyms (hey, I remember the first infinintely-recursive acronym I ran across, GNU, back when it was still pretty new in 1984).
I figured they are harmless because they are all defined within a type with a well defined name so it should be clear what they are for. However, for the record:
CCP stands for Ccpp_Constituent_Properties_t
and
CCPT stands for Ccpp_Constituent_prop_Ptr_T

These acronyms should not show up outside this module (can they even be called from outside?).

climbfuji · 2023-11-09T03:55:43Z

+
+      if (associated(this%prop)) then
+         call this%prop%vertical_dimension(dimname)
+         is_2d = len_trim(dimname) == 0


Another point: While many models move to one horizontal dimension and one vertical dimension, we all have a basic understanding that 2d is something on a surface and 3d something in a volume, therefore this makes sense.

gold2718 · 2023-11-10T13:57:24Z

My only real concern is the newly added debug_on feature

I second this. I think the name probably came from the name of the logging level but it is certainly verbose!

gold2718

9db9477 looks good, thanks!

climbfuji · 2023-11-27T17:56:17Z

@peverwhee We were wondering in our tag up today when/if you were planning to make the debug_on() --> verbose() change so that we can approve and merge?

peverwhee · 2023-11-27T20:42:11Z

@climbfuji Yes! I will try to finish up addressing the remaining comments tomorrow.

climbfuji

Thanks for making the debug_on -> verbose change!

Courtney Peverley and others added 6 commits August 21, 2023 13:33

initial stab at constituents-only PR

69e2bf9

cleanup and fixes

52e5985

add is_scheme_constituent interface

a5aa37b

fix tests and add is_thermo_active property

9897870

add ddt testing and update constituent standard names

73ef0a3

add testing files

071687e

peverwhee requested review from dustinswales and mwaxmonsky September 1, 2023 16:56

peverwhee self-assigned this Sep 1, 2023

peverwhee requested review from climbfuji and gold2718 as code owners September 1, 2023 16:56

dustinswales approved these changes Oct 3, 2023

View reviewed changes

Comment thread src/ccpp_constituent_prop_mod.meta

Comment thread test/unit_tests/sample_host_files/ddt1.meta

merge up

d8b7b04

peverwhee requested review from grantfirl and mkavulich as code owners October 12, 2023 22:24

fix merge

5560875

gold2718 requested changes Oct 15, 2023

View reviewed changes

mkavulich reviewed Oct 16, 2023

View reviewed changes

mwaxmonsky reviewed Oct 24, 2023

View reviewed changes

Courtney Peverley added 3 commits October 31, 2023 22:27

address reviewer comments

370e78d

merge feature/capgen

e9e70b7

fix tests

ac18c44

peverwhee requested review from gold2718, mkavulich and mwaxmonsky November 1, 2023 04:47

gold2718 reviewed Nov 1, 2023

View reviewed changes

gold2718 requested changes Nov 1, 2023

View reviewed changes

gold2718 reviewed Nov 1, 2023

View reviewed changes

Comment thread src/ccpp_constituent_prop_mod.F90 Outdated

address reviewer comments

9db9477

peverwhee requested a review from gold2718 November 8, 2023 22:19

climbfuji reviewed Nov 9, 2023

View reviewed changes

gold2718 approved these changes Nov 10, 2023

View reviewed changes

change debug_on subroutine in framework_env to verbose property

4c1807c

peverwhee requested a review from climbfuji November 29, 2023 03:51

climbfuji approved these changes Nov 29, 2023

View reviewed changes

peverwhee merged commit e86d0a7 into NCAR:feature/capgen Nov 29, 2023

peverwhee mentioned this pull request May 12, 2025

Some arrays need accompanying properties array #282

Closed

peverwhee deleted the constituents branch November 18, 2025 23:42

Conversation

peverwhee commented Sep 1, 2023

Summary

Description

Uh oh!

dustinswales left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dustinswales commented Oct 3, 2023

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkavulich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peverwhee Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mwaxmonsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peverwhee Nov 1, 2023 •

edited

Loading

climbfuji Nov 9, 2023 •

edited

Loading