NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 64 forks source link

Handle dimensions for promoted variables #562

Closed peverwhee closed 4 months ago

peverwhee commented 5 months ago

Description

Handle edge case where variable dimensions weren't getting added to the init phase calling list for allocation of promoted, suite-level variables.

Also includes logic to use horizontal_dimension in allocate statement for run-time only variables (with dimension horizontal_loop_extent)

Changes

User interface changes?: No

Fixes

closes #559

Testing

Test added for runtime-only promoted variables

dustinswales commented 4 months ago

@peverwhee Maybe (probably) I don't understand the promotion business in Capgen, but looking at the test code I'm confused on the spirit of these changes. There are two physics groups, both have schemes that use the promote_this_variable_to_suite variable, but in the physics1 group it is two-dimensional, whereas in the physics2 group it is one-dimensional. Why we would be doing something like this? How in the CCPP world is this allowable? (The host metadata is either 1D or 2D, so how do we parse this situation w/o error?)

gold2718 commented 4 months ago

@dustinswales

@peverwhee Maybe (probably) I don't understand the promotion business in Capgen, but looking at the test code I'm confused on the spirit of these changes. There are two physics groups, both have schemes that use the promote_this_variable_to_suite variable, but in the physics1 group it is two-dimensional, whereas in the physics2 group it is one-dimensional. Why we would be doing something like this? How in the CCPP world is this allowable? (The host metadata is either 1D or 2D, so how do we parse this situation w/o error?)

To understand why this works, you have to know / learn about another corner of capgen. From the point of view of physics2, the variable, promote_this_variable_to_suite exists and has the required dimension so no worries :) Note that temp_adjust is basically a 2-D (one level) scheme. When a scheme like this is called where the available data is 3-D, capgen automatically inserts a loop over the vertical dimension so that the 2-D scheme is called for each level. That is why this works (with or without promotion). However, if you tried to reverse physics1 and physics2, capgen should complain because the data needed by physics1 (variables with vertical_layer_dimension) is not present in the interface variables with the necessary dimensions. This should be tested if it is not already.

I hope this helps and does not just make everything more confusing.

dustinswales commented 4 months ago

@dustinswales

@peverwhee Maybe (probably) I don't understand the promotion business in Capgen, but looking at the test code I'm confused on the spirit of these changes. There are two physics groups, both have schemes that use the promote_this_variable_to_suite variable, but in the physics1 group it is two-dimensional, whereas in the physics2 group it is one-dimensional. Why we would be doing something like this? How in the CCPP world is this allowable? (The host metadata is either 1D or 2D, so how do we parse this situation w/o error?)

To understand why this works, you have to know / learn about another corner of capgen. From the point of view of physics2, the variable, promote_this_variable_to_suite exists and has the required dimension so no worries :) Note that temp_adjust is basically a 2-D (one level) scheme. When a scheme like this is called where the available data is 3-D, capgen automatically inserts a loop over the vertical dimension so that the 2-D scheme is called for each level. That is why this works (with or without promotion). However, if you tried to reverse physics1 and physics2, capgen should complain because the data needed by physics1 (variables with vertical_layer_dimension) is not present in the interface variables with the necessary dimensions. This should be tested if it is not already.

I hope this helps and does not just make everything more confusing.

@gold2718 Thanks for the explanation. Makes sense. @peverwhee Personally, it helps me to see the autogenerated code, so I think it's a good idea to insert some autogenerated code snippets with the PR (next PR, no need this time)

peverwhee commented 4 months ago

Merged into #549