Skip to content

Handle dimensions for promoted variables#562

Closed
peverwhee wants to merge 3 commits into
NCAR:mainfrom
peverwhee:group-fix
Closed

Handle dimensions for promoted variables#562
peverwhee wants to merge 3 commits into
NCAR:mainfrom
peverwhee:group-fix

Conversation

@peverwhee
Copy link
Copy Markdown
Collaborator

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

  • scripts/ccpp_suite.py: Add dimensions of promoted variable to init group calling list; use horizontal_dimension instead of horizontal_loop_extent if necessary
  • scripts/suite_objects.py: Use horizontal_dimension instead of horizonal_loop_extent for allocation of promoted, suite-level variables
  • test/capgen_test/*: add run-time only variables that need to be promoted and allocated (in run phase of two separate groups and used nowhere else)

User interface changes?: No

Fixes

closes #559

Testing

Test added for runtime-only promoted variables

Copy link
Copy Markdown
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of questions about code

Comment thread scripts/ccpp_suite.py Outdated
Comment on lines +343 to +350
else:
# Add dimensions if they're not already there
group.add_variable_dimensions(var, [],
adjust_intent=True,
to_dict=group.call_list)
# end if
# end if
# end for
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a case for this? Is it tested (capgen test seems to run fine without it)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case would be a promoted variable with no horizontal dimension, but the existing dimension(s) do still need to be added to the calling list for the init routine.

That said, thanks to your other suggestion, I reworked the code so this "else" is no more. I also altered the capgen_test to ensure promotion/allocation works as expected for a variable with no horizontal dimension

Comment thread scripts/suite_objects.py Outdated
Comment on lines +2244 to +2245
else:
dims = [new_horiz_dim]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the only other possibility? No other dimensions allowed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very good question.

Updated the logic in ccpp_suite to replace the horizontal dimension where it is and not remove other dimensions! This logic is now gone though, as I discovered it was unnecessary (the logic in ccpp_suite to promote variables handles the dimensions)

@dustinswales
Copy link
Copy Markdown
Member

@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
Copy link
Copy Markdown
Collaborator

@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
Copy link
Copy Markdown
Member

@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 peverwhee requested a review from gold2718 May 16, 2024 17:55
Copy link
Copy Markdown
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks!

Copy link
Copy Markdown
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and I think I actually understand what it is doing ;-)

@peverwhee
Copy link
Copy Markdown
Collaborator Author

Merged into #549

@peverwhee peverwhee closed this May 20, 2024
@peverwhee peverwhee deleted the group-fix branch November 18, 2025 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suite-level variables edge case for when variable is used only in run phases

4 participants