Add support (and tests) for optional arguments in ccpp_prebuild #552
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -198,16 +198,28 @@ def read_new_metadata(filename, module_name, table_name, scheme_name = None, sub | |||||||||||||||||||||||||||||||||||||
| # *DH 2020-05-26 | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if not new_var.get_prop_value('active'): | ||||||||||||||||||||||||||||||||||||||
| raise Exception("Unexpected result: no active attribute received from capgen metadata parser for {} / {}".format(standard_name,table_name)) | ||||||||||||||||||||||||||||||||||||||
| elif scheme_name and not new_var.get_prop_value('active').lower() == '.true.': | ||||||||||||||||||||||||||||||||||||||
| raise Exception("Scheme variable {} in table {} has metadata attribute active={}, which is not allowed".format( | ||||||||||||||||||||||||||||||||||||||
| standard_name, table_name, new_var.get_prop_value('active').lower())) | ||||||||||||||||||||||||||||||||||||||
| elif new_var.get_prop_value('active').lower() == '.true.': | ||||||||||||||||||||||||||||||||||||||
| active = 'T' | ||||||||||||||||||||||||||||||||||||||
| elif new_var.get_prop_value('active').lower() == '.false.': | ||||||||||||||||||||||||||||||||||||||
| active = 'F' | ||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||
| # Replace multiple whitespaces, preserve case | ||||||||||||||||||||||||||||||||||||||
| active = ' '.join(new_var.get_prop_value('active').split()) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
Comment on lines
200
to
212
There was a problem hiding this comment. Because we use the active_prop = new_var.get_prop_value('active')
if not active_prop:
raise Exception(f"Unexpected result: no active attribute received from capgen metadata parser for {standard_name} / {table_name}")
active_prop = active_prop.lower()
if active_prop == '.true':
active = 'T'
elif active_prop == '.false':
active = 'F'
else:
active = ' '.join(active_prop.split())There was a problem hiding this comment. Thanks, @mwaxmonsky I do agree but given that the regression testing with the UFS is about halfway through I'll refrain from making this change. That code will go away as soon as we move to capgen anyway :-) |
||||||||||||||||||||||||||||||||||||||
| if not new_var.get_prop_value('optional') in [False, True]: | ||||||||||||||||||||||||||||||||||||||
| raise Exception("Unexpected result: no optional attribute received from metadata parser for {} / {}".format(standard_name,table_name)) | ||||||||||||||||||||||||||||||||||||||
| elif not scheme_name and new_var.get_prop_value('optional'): | ||||||||||||||||||||||||||||||||||||||
| raise Exception("Host variable {} in table {} has metadata attribute optional={}, which is not allowed".format( | ||||||||||||||||||||||||||||||||||||||
| standard_name,table_name, new_var.get_prop_value('optional').lower())) | ||||||||||||||||||||||||||||||||||||||
| elif new_var.get_prop_value('optional'): | ||||||||||||||||||||||||||||||||||||||
| optional = 'T' | ||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||
| optional = 'F' | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+213
to
+221
There was a problem hiding this comment. Similarly for the optional property:
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # DH* 20210812 | ||||||||||||||||||||||||||||||||||||||
| # Workaround for Fortran DDTs incorrectly having the type of | ||||||||||||||||||||||||||||||||||||||
| # the DDT copied into the kind attribute in parse_metadata_file | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -228,6 +240,7 @@ def read_new_metadata(filename, module_name, table_name, scheme_name = None, sub | |||||||||||||||||||||||||||||||||||||
| kind = kind, | ||||||||||||||||||||||||||||||||||||||
| intent = new_var.get_prop_value('intent'), | ||||||||||||||||||||||||||||||||||||||
| active = active, | ||||||||||||||||||||||||||||||||||||||
| optional = optional, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| # Check for duplicates in same table | ||||||||||||||||||||||||||||||||||||||
| if standard_name in metadata.keys(): | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,8 @@ def __init__(self, **kwargs): | |
| self._kind = None | ||
| self._intent = None | ||
| self._active = None | ||
| self._optional = None | ||
| self._pointer = False | ||
| self._target = None | ||
| self._actions = { 'in' : None, 'out' : None } | ||
| for key, value in kwargs.items(): | ||
|
|
@@ -134,6 +136,30 @@ def active(self, value): | |
| raise ValueError('Invalid value {0} for variable property active, must be a string'.format(value)) | ||
| self._active = value | ||
|
|
||
| @property | ||
| def optional(self): | ||
| '''Get the optional attribute of the variable.''' | ||
| return self._optional | ||
|
|
||
| @optional.setter | ||
| def optional(self, value): | ||
| if not isinstance(value, str): | ||
| raise ValueError('Invalid value {0} for variable property optional, must be a string'.format(value)) | ||
| self._optional = value | ||
|
|
||
| # Pointer is not set by parsing metadata attributes, but by mkstatic. | ||
| # This is a quick and dirty solution! | ||
| @property | ||
| def pointer(self): | ||
| '''Get the pointer attribute of the variable.''' | ||
| return self._pointer | ||
|
|
||
| @pointer.setter | ||
| def pointer(self, value): | ||
| if not isinstance(value, bool): | ||
| raise ValueError('Invalid value {0} for variable property pointer, must be a logical'.format(value)) | ||
| self._pointer = value | ||
|
|
||
| @property | ||
| def target(self): | ||
| '''Get the target of the variable.''' | ||
|
|
@@ -270,62 +296,128 @@ def print_def_intent(self, metadata): | |
| dictionary to resolve lower bounds for array dimensions.''' | ||
| # Resolve dimensisons to local names using undefined upper bounds (assumed shape) | ||
| dimstring = self.dimstring_local_names(metadata, assume_shape = True) | ||
| # It is an error for host model variables to have the optional attribute in the metadata | ||
|
There was a problem hiding this comment. I need to check if hosts having the optional attribute in Capgen is an error. I don't recall adding this, but maybe it's in there somewhere. @gold2718 Do you know by chance? There was a problem hiding this comment. I don't see where you could have put it. Is it possible that the distinction is really about not allowing the I'm not sure where this leaves DDTs. For a host DDT, can an element have the Thoughts? There was a problem hiding this comment. Yes, this is about the metadata only. The rule is quite simple (if I understood it correctly):
prebuild checks for 1-5 in its own layer after receiving the metadata from capgen. This is because we only harvest information on host metadata and scheme metadata separately (except for passing known host DDTs) and do not rely on the metadata comparison host <--> model from capgen. I will note that at this point, prebuild spits out a warning for 6 if it finds a variable with optional being true but no non-default active attribute in the model. I used this to clean up the metadata, knowing that the current ccpp physics are tied to the GFS data structures used by UFS, SCM, NEPTUNE. Regarding your pointer question. That gets tricky. The only way out I can see here is that if a host ddt has an active attribute other than the default true, all it's members (if declared explicitly in the metadata) need that to have active to, with the allocation condition being that of the DDT (if the member is always allocated when the DDT is allocated) OR a Fortran logical && of the DDTs allocation condition and its own. We need to be explicit here and have both allocation conditions in the member var's active attribute instead of implicitly inheriting it from the DDT. That would greatly confuse the physics/model developers and users. There was a problem hiding this comment. @climbfuji, I think your rules 1-6 plus thoughts on DDTs all make sense. We should make sure all of these are tested in capgen, do we need a new issue for that? There was a problem hiding this comment. Yes, that would be good. I'll work with @dustinswales on that There was a problem hiding this comment. 1,2,5, and 6 are applied In the Capgen implementation, with the exception of 3 and 4. I will add these soon, and revisit the DDTs. |
||
| if self.optional == 'T': | ||
| error_message = "This routine should only be called for host model variables" + \ | ||
| " that cannot be optional, but got self.optional=T" | ||
| raise Exception(error_message) | ||
| # If the host variable is potentially unallocated, add optional and target to variable declaration | ||
| elif not self.active == 'T': | ||
| optional = ', optional, target' | ||
| else: | ||
| optional = '' | ||
| # | ||
| if self.type in STANDARD_VARIABLE_TYPES: | ||
| if self.kind: | ||
| str = "{s.type}({s._kind}), intent({s.intent}){optional} :: {s.local_name}{dimstring}" | ||
| else: | ||
| str = "{s.type}, intent({s.intent}){optional} :: {s.local_name}{dimstring}" | ||
| else: | ||
| if self.kind: | ||
| error_message = "Generating variable definition statements for derived types with" + \ | ||
| " kind attributes not implemented; variable: {0}".format(self.standard_name) | ||
| raise Exception(error_message) | ||
| else: | ||
| str = "type({s.type}), intent({s.intent}){optional} :: {s.local_name}{dimstring}" | ||
| return str.format(s=self, optional=optional, dimstring=dimstring) | ||
|
|
||
| def print_def_local(self, metadata): | ||
| '''Print the definition line for the variable, assuming it is a local variable.''' | ||
| # It is an error for local variables to have the active attribute | ||
| if not self.active == 'T': | ||
| error_message = "This routine should only be called for local variables" + \ | ||
| " that cannot have an active attribute other than the" +\ | ||
| " default T, but got self.active=T" | ||
| raise Exception(error_message) | ||
|
|
||
| # If it is a pointer, everything is different! | ||
| if self.pointer: | ||
| if self.type in STANDARD_VARIABLE_TYPES: | ||
| if self.kind: | ||
| if self.rank: | ||
| str = "{s.type}({s._kind}), dimension{s.rank}, pointer :: p => null()" | ||
| else: | ||
| str = "{s.type}({s._kind}), pointer :: p => null()" | ||
| else: | ||
| if self.rank: | ||
| str = "{s.type}, dimension{s.rank}, pointer :: p => null()" | ||
| else: | ||
| str = "{s.type}, pointer :: p => null()" | ||
| else: | ||
| if self.kind: | ||
| error_message = "Generating variable definition statements for derived types with" + \ | ||
| " kind attributes not implemented; variable: {0}".format(self.standard_name) | ||
| raise Exception(error_message) | ||
| else: | ||
| if self.rank: | ||
| str = "type({s.type}), dimension{s.rank}, pointer :: p => null()" | ||
| else: | ||
| str = "type({s.type}), pointer :: p => null()" | ||
| return str.format(s=self) | ||
| else: | ||
| # If the host variable is potentially unallocated, the active attribute is | ||
| # also set accordingly for the local variable; add target to variable declaration | ||
| if self.optional == 'T': | ||
| target = ', target' | ||
| else: | ||
| target = '' | ||
| if self.type in STANDARD_VARIABLE_TYPES: | ||
| if self.kind: | ||
| if self.rank: | ||
| str = "{s.type}({s._kind}), dimension{s.rank}, allocatable{target} :: {s.local_name}" | ||
| else: | ||
| str = "{s.type}({s._kind}){target} :: {s.local_name}" | ||
| else: | ||
| if self.rank: | ||
| str = "{s.type}, dimension{s.rank}, allocatable{target} :: {s.local_name}" | ||
| else: | ||
| str = "{s.type}{target} :: {s.local_name}" | ||
| else: | ||
| if self.kind: | ||
| error_message = "Generating variable definition statements for derived types with" + \ | ||
| " kind attributes not implemented; variable: {0}".format(self.standard_name) | ||
| raise Exception(error_message) | ||
| else: | ||
| if self.rank: | ||
| str = "type({s.type}), dimension{s.rank}, allocatable{target} :: {s.local_name}" | ||
| else: | ||
| str = "type({s.type}){target} :: {s.local_name}" | ||
| return str.format(s=self, target=target) | ||
|
|
||
| def print_debug(self): | ||
| '''Print the data retrieval line for the variable.''' | ||
| # Scheme variables don't have the active attribute | ||
| if 'SCHEME' in self.container: | ||
| str='''Contents of {s} (* = mandatory for compatibility): | ||
| standard_name = {s.standard_name} * | ||
| long_name = {s.long_name} | ||
| units = {s.units} * | ||
| local_name = {s.local_name} | ||
| type = {s.type} * | ||
| dimensions = {s.dimensions} | ||
| rank = {s.rank} * | ||
| kind = {s.kind} * | ||
| intent = {s.intent} | ||
| optional = {s.optional} | ||
| target = {s.target} | ||
| container = {s.container} | ||
| actions = {s.actions}''' | ||
| # Host model variables don't have the optional attribute | ||
| else: | ||
| str='''Contents of {s} (* = mandatory for compatibility): | ||
| standard_name = {s.standard_name} * | ||
| long_name = {s.long_name} | ||
| units = {s.units} * | ||
| local_name = {s.local_name} | ||
| type = {s.type} * | ||
| dimensions = {s.dimensions} | ||
| rank = {s.rank} * | ||
| kind = {s.kind} * | ||
| intent = {s.intent} | ||
| active = {s.active} | ||
| target = {s.target} | ||
| container = {s.container} | ||
| actions = {s.actions}''' | ||
| return str.format(s=self) | ||
|
|
||
| class CapsMakefile(object): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really temporary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is only there because all models using ccpp_prebuild are in lockstep in terms of allocating or not allocating variables. I used this to check that there are no unnecessary
optionalvariables in the physics.But going forward, CESM may declare some variables as potentially allocated, whereas the UFS doesn't, or vice versa. We still need the schemes make this variable
optional, but the UFS would have theactiveattribute.