Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfixes for multiple groups and scalar variable transforms #542

Merged
merged 5 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion scripts/ccpp_suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from parse_tools import read_xml_file, validate_xml_file, find_schema_version
from parse_tools import init_log, set_log_to_null
from suite_objects import CallList, Group, Scheme
from metavar import CCPP_LOOP_VAR_STDNAMES

# pylint: disable=too-many-lines

Expand Down Expand Up @@ -286,13 +287,16 @@ def find_variable(self, standard_name=None, source_var=None,
loop_subst=loop_subst)
if var is None:
# No dice? Check for a group variable which can be promoted
if standard_name in self.__gvar_stdnames:
# Don't promote loop standard names
if (standard_name in self.__gvar_stdnames and standard_name
not in CCPP_LOOP_VAR_STDNAMES):
group = self.__gvar_stdnames[standard_name]
var = group.find_variable(standard_name=standard_name,
source_var=source_var,
any_scope=False,
search_call_list=srch_clist,
loop_subst=loop_subst)

if var is not None:
# Promote variable to suite level
# Remove this entry to avoid looping back here
Expand Down
4 changes: 2 additions & 2 deletions scripts/host_cap.py
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ def write_host_cap(host_model, api, module_name, output_dir, run_env):
# Look for any loop-variable mismatch
for suite in api.suites:
spart_list = suite_part_list(suite, stage)
for spart in spart_list:
for _, spart in sorted(enumerate(spart_list)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does the sorted(enumerate(spart_list)) buy here? What is wrong with just going through the spart_list?
If it is some sort new of 'best practice', why here and line 533 but not line 566?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good question! I had added "sorted" to line 533 on a different branch to keep the use statements in a consistent order, which broke when you use multiple groups (thereby invalidating the whole point of the change of course), so that line became "sorted(enumerate(" and then I just migrated the full change to the other loop for... reasons that made sense at the time.

I'll revert these changes here and will bring in the sorted changes (there are are more than just this one) in a future PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reverted

spart_args = spart.call_list.variable_list()
for sp_var in spart_args:
stdname = sp_var.get_prop_value('standard_name')
Expand Down Expand Up @@ -530,7 +530,7 @@ def write_host_cap(host_model, api, module_name, output_dir, run_env):
for suite in api.suites:
mspc = (max_suite_len - len(suite.module))*' '
spart_list = suite_part_list(suite, stage)
for spart in spart_list:
for _, spart in sorted(enumerate(spart_list)):
stmt = "use {}, {}only: {}"
cap.write(stmt.format(suite.module, mspc, spart.name), 2)
# End for
Expand Down
2 changes: 1 addition & 1 deletion scripts/metavar.py
Original file line number Diff line number Diff line change
Expand Up @@ -1728,9 +1728,9 @@ def add_variable_dimensions(self, var, ignore_sources, to_dict=None,
err_ret += f"{self.name}: "
err_ret += f"Cannot find variable for dimension, {dimname}, of {vstdname}{ctx}"
if dvar:
err_ret += f"\nFound {lname} from excluded source, '{dvar.source.ptype}'{dctx}"
lname = dvar.get_prop_value('local_name')
dctx = context_string(dvar.context)
err_ret += f"\nFound {lname} from excluded source, '{dvar.source.ptype}'{dctx}"
# end if
# end if
# end if
Expand Down
25 changes: 22 additions & 3 deletions scripts/suite_objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -1535,6 +1535,16 @@ def add_var_transform(self, var, compat_obj, vert_dim):

# If needed, modify vertical dimension for vertical orientation flipping
_, vdim = find_vertical_dimension(var.get_dimensions())
if vdim >= 0:
vdim_name = vert_dim.split(':')[-1]
group_vvar = self.__group.call_list.find_variable(vdim_name)
vname = group_vvar.get_prop_value('local_name')
lindices[vdim] = '1:'+vname
rindices[vdim] = '1:'+vname
if compat_obj.has_vert_transforms:
rindices[vdim] = vname+':1:-1'
# end if
# end if
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do vertical dimensions have to begin at 1? Should we use variable starting point if provided?

Suggested change
vdim_name = vert_dim.split(':')[-1]
group_vvar = self.__group.call_list.find_variable(vdim_name)
vname = group_vvar.get_prop_value('local_name')
lindices[vdim] = '1:'+vname
rindices[vdim] = '1:'+vname
if compat_obj.has_vert_transforms:
rindices[vdim] = vname+':1:-1'
# end if
# end if
vdims = vert_dim.split(':')
vdim_name = vdims[-1]
group_vvar = self.__group.call_list.find_variable(vdim_name)
if group_vvar is None:
raise CCPPError(f"add_var_transform: Cannot find dimension variable, {vdim_name}")
# end if
vname = group_vvar.get_prop_value('local_name')
if len(vdims) == 2:
sdim_name = vdims[0]
group_vvar = self.find_variable(sdim_name)
if group_vvar is None:
raise CCPPError(f"add_var_transform: Cannot find dimension variable, {sdim_name}")
# end if
sname = group_vvar.get_prop_value('local_name')
else:
sname = '1'
# end if
lindices[vdim] = sname+':'+vname
if compat_obj.has_vert_transforms:
rindices[vdim] = vname+':'+sname+':-1'
else:
rindices[vdim] = sname+':'+vname
# end if
# end if

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

smart! changed.

vdim_name = vert_dim.split(':')[-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this section (1548 - 1554) a repeat of the stuff that was moved inside the if vdim >= 0: (line 1538)? If so, should a test have caught this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch - the only code that definitely needed to be moved inside the if block was vdims = vert_dim.split(':') so a test wouldn't have caught this. that said, I did add some more testing that would've caught the original bug.

group_vvar = self.__group.call_list.find_variable(vdim_name)
vname = group_vvar.get_prop_value('local_name')
Expand Down Expand Up @@ -1614,20 +1624,25 @@ def write(self, outfile, errcode, errmsg, indent):
for (var, internal_var) in self.__var_debug_checks:
stmt = self.write_var_debug_check(var, internal_var, cldicts, outfile, errcode, errmsg, indent+1)
# Write any reverse (pre-Scheme) transforms.
outfile.write('! Compute reverse (pre-scheme) transforms', indent+1)
if len(self.__reverse_transforms) > 0:
outfile.write('! Compute reverse (pre-scheme) transforms', indent+1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be:

Suggested change
outfile.write('! Compute reverse (pre-scheme) transforms', indent+1)
outfile.comment('Compute reverse (pre-scheme) transforms', indent+1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

# end if
for (dummy, var, rindices, lindices, compat_obj) in self.__reverse_transforms:
tstmt = self.write_var_transform(var, dummy, rindices, lindices, compat_obj, outfile, indent+1, False)
# end for
# Write the scheme call.
stmt = 'call {}({})'
outfile.write('',indent+1)
outfile.write('! Call scheme', indent+1)
outfile.write(stmt.format(self.subroutine_name, my_args), indent+1)
# Write any forward (post-Scheme) transforms.
outfile.write('',indent+1)
outfile.write('! Compute forward (post-scheme) transforms', indent+1)
if len(self.__forward_transforms) > 0:
outfile.write('! Compute forward (post-scheme) transforms', indent+1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here:

Suggested change
outfile.write('! Compute forward (post-scheme) transforms', indent+1)
outfile.comment('Compute forward (post-scheme) transforms', indent+1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed.

# end if
for (var, dummy, lindices, rindices, compat_obj) in self.__forward_transforms:
tstmt = self.write_var_transform(var, dummy, rindices, lindices, compat_obj, outfile, indent+1, True)
#
# end for
outfile.write('', indent)
outfile.write('end if', indent)

Expand Down Expand Up @@ -1724,6 +1739,10 @@ def analyze(self, phase, group, scheme_library, suite_vars, level):
local_dim = group.call_list.find_variable(standard_name=dim_name,
any_scope=False)
# end if
# If not found, check the suite level
if local_dim is None:
local_dim = group.suite.find_variable(standard_name=dim_name)
# end if
if local_dim is None:
emsg = 'No variable found for vertical loop dimension {}'
raise ParseInternalError(emsg.format(self._dim_name))
Expand Down
18 changes: 14 additions & 4 deletions scripts/var_props.py
Original file line number Diff line number Diff line change
Expand Up @@ -976,8 +976,13 @@ def forward_transform(self, lvar_lname, rvar_lname, rvar_indices, lvar_indices,
"vertical_interface_dimension").
"""
# Dimension transform (Indices handled externally)
rhs_term = f"{rvar_lname}({','.join(rvar_indices)})"
lhs_term = f"{lvar_lname}({','.join(lvar_indices)})"
if len(rvar_indices) == 0:
rhs_term = f"{rvar_lname}"
lhs_term = f"{lvar_lname}"
else:
rhs_term = f"{rvar_lname}({','.join(rvar_indices)})"
lhs_term = f"{lvar_lname}({','.join(lvar_indices)})"
# end if

if self.has_kind_transforms:
kind = self.__kind_transforms[1]
Expand Down Expand Up @@ -1016,8 +1021,13 @@ def reverse_transform(self, lvar_lname, rvar_lname, rvar_indices, lvar_indices,
"vertical_interface_dimension").
"""
# Dimension transforms (Indices handled externally)
lhs_term = f"{lvar_lname}({','.join(lvar_indices)})"
rhs_term = f"{rvar_lname}({','.join(rvar_indices)})"
if len(rvar_indices) == 0:
rhs_term = f"{rvar_lname}"
lhs_term = f"{lvar_lname}"
else:
lhs_term = f"{lvar_lname}({','.join(lvar_indices)})"
rhs_term = f"{rvar_lname}({','.join(rvar_indices)})"
# end if

if self.has_kind_transforms:
kind = self.__kind_transforms[0]
Expand Down
4 changes: 3 additions & 1 deletion test/capgen_test/temp_suite.xml
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>

<suite name="temp_suite" version="1.0">
<group name="physics">
<group name="physics1">
<scheme>temp_set</scheme>
</group>
<group name="physics2">
<scheme>temp_calc_adjust</scheme>
<scheme>temp_adjust</scheme>
</group>
Expand Down
3 changes: 2 additions & 1 deletion test/capgen_test/test_host.F90
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,8 @@ program test

implicit none

character(len=cs), target :: test_parts1(1) = (/ 'physics ' /)
character(len=cs), target :: test_parts1(2) = (/ 'physics1 ', &
'physics2 ' /)
character(len=cs), target :: test_parts2(1) = (/ 'data_prep ' /)
character(len=cm), target :: test_invars1(6) = (/ &
'potential_temperature ', &
Expand Down
Loading