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

Capgen in SCM: Differing source and module name #637

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion scripts/ccpp_suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,8 @@ def __init__(self, sdfs, host_model, scheme_headers, run_env):
self.__ddt_lib = DDTLibrary('{}_api'.format(self.host_model.name),
run_env, ddts=all_ddts)
for header in [d for d in scheme_headers if d.header_type != 'ddt']:
if header.header_type != 'scheme':
# DJS2024: Schemes and modules with DDTs?
if header.header_type != 'scheme' and header.header_type != 'module':
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@climbfuji Here's another thing I needed to add. Capgen wasn't happy with modules that held DDT definitions (e.g. radlw_param.f).
I convinced myself that this is not intentional and probably due to a lack of testing Capgen with respect to DDTs.

errmsg = "{} is an unknown CCPP API metadata header type, {}"
raise CCPPError(errmsg.format(header.title, header.header_type))
# end if
Expand Down
56 changes: 53 additions & 3 deletions scripts/metadata_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,51 @@ def parse_metadata_file(filename, known_ddts, run_env, skip_ddt_check=False):

########################################################################

def find_module_name(filename):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am surprised that this isn't already done by the parser. Is that because everything is based on the assumption filename=modulename?

Copy link
Collaborator Author

@dustinswales dustinswales Feb 3, 2025

Choose a reason for hiding this comment

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

Yeah. Metadata tables for new DDTs default to the source filename (e.g. here). Not the best.
This is a problem for DDTs in two instances, a) DDTs housed in differing module/file names and b) DDTs in files with multiple modules.
We don't have any of the latter in the SCM/UFS, so I haven't gone down that rabbit hole.

"""Find the module name from module header in <filename>"""
module_name = ''
if os.path.isfile(filename):
with open(filename, 'r') as infile:
fin_lines = infile.readlines()
# end with
num_lines = len(fin_lines)
context = ParseContext(linenum=1, filename=filename)
while context.line_num <= num_lines:
if MetadataTable.table_start(fin_lines[context.line_num - 1]):
found_start = False
while not found_start:
line = fin_lines[context.line_num].strip()
context.line_num += 1
if line and (line[0] == '['):
found_start = True
elif line:
props = _parse_config_line(line, context)
for prop in props:
# Look for name property
key = prop[0].strip().lower()
value = prop[1].strip()
if key == 'name' :
name = value
if key == 'type' :
if (value == 'module') or (value == 'scheme'):
module_name = name
break
# end if
# end for
# end if
if context.line_num > num_lines:
break
# end if
# end while
else:
context.line_num += 1
# end if
# end while
# end if
return module_name

########################################################################

def find_scheme_names(filename):
"""Find and return a list of all the physics scheme names in
<filename>. A scheme is identified by its ccpp-table-properties name.
Expand Down Expand Up @@ -813,9 +858,14 @@ def __init_from_file(self, table_name, table_type, known_ddts, run_env, skip_ddt
if self.header_type == "ddt":
known_ddts.append(self.title)
# end if
# We need a default module if none was listed
if self.module is None:
self.__module_name = self._default_module()
# We need a default module if none was listed.
# DJS2024: If module_name not provided through initialization, try
# to find module_name from the metadata. Otherwise, use file name
# as module_name (default).
if (self.__module_name == None):
self.__module_name = find_module_name(self.__pobj.filename)
if (self.__module_name == ''):
self.__module_name = self._default_module()
# end if
# Initialize our ParseSource parent
super().__init__(self.title, self.header_type, self.__pobj)
Expand Down
2 changes: 1 addition & 1 deletion test/var_compatibility_test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ get_filename_component(CCPP_ROOT "${TEST_ROOT}" DIRECTORY)
#
#------------------------------------------------------------------------------
LIST(APPEND SCHEME_FILES "var_compatibility_files.txt")
LIST(APPEND HOST_FILES "test_host_data" "test_host_mod")
LIST(APPEND HOST_FILES "module_rad_ddt" "test_host_data" "test_host_mod")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gold2718 @peverwhee @climbfuji Design philosophy question below.

When a physics scheme has a native DDT that the host needs to be aware of, the file containing the DDT needs to be at the top of both the scheme_files and host_files list (e.g. host_files and scheme_files in CCPP SCM). In the SCM/UFS, we also do the inverse and use host defined DDTs within Interstitial physics schemes, so there is a need for a pool of DDTs available to both the host and the schemes.

Would it make sense to have a third class of files provided to Capgen, containing the DDT typedefs, that are processed before the scheme and host files?
I'm fine with having to define the host/scheme lists with the typedefs at the top, just curious to hear your thoughts on this? (I'm also not volunteering to do this!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That third class would be a mix of scheme and host files. Something like this exists in prebuild, see for example https://github.com/NOAA-EMC/fv3atm/blob/2c902a670e89416ef49254c827e8ba45a68ce596/ccpp/config/ccpp_prebuild_config.py#L12

LIST(APPEND SUITE_FILES "var_compatibility_suite.xml")
# HOST is the name of the executable we will build.
# We assume there are files ${HOST}.meta and ${HOST}.F90 in CMAKE_SOURCE_DIR
Expand Down
28 changes: 28 additions & 0 deletions test/var_compatibility_test/module_rad_ddt.F90
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
module mod_rad_ddt
USE ccpp_kinds, ONLY: kind_phys
implicit none

public ty_rad_lw, ty_rad_sw

!> \section arg_table_mod_rad_ddt Argument Table
!! \htmlinclude arg_table_mod_rad_ddt.html
!!

!> \section arg_table_ty_rad_lw Argument Table
!! \htmlinclude arg_table_ty_rad_lw.html
!!
type ty_rad_lw
real(kind_phys) :: sfc_up_lw
real(kind_phys) :: sfc_down_lw
end type ty_rad_lw

!> \section arg_table_ty_rad_sw Argument Table
!! \htmlinclude arg_table_ty_rad_sw.html
!!
type ty_rad_sw
real(kind_phys) :: sfc_up_sw
real(kind_phys) :: sfc_down_sw
end type ty_rad_sw

end module mod_rad_ddt

45 changes: 45 additions & 0 deletions test/var_compatibility_test/module_rad_ddt.meta
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
[ccpp-table-properties]
name = mod_rad_ddt
type = module
[ccpp-arg-table]
name = mod_rad_ddt
type = module

[ccpp-table-properties]
name = ty_rad_lw
type = ddt
dependencies =
[ccpp-arg-table]
name = ty_rad_lw
type = ddt
[ sfc_up_lw ]
standard_name = surface_upwelling_longwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys
[ sfc_down_lw ]
standard_name = surface_downwelling_longwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys

[ccpp-table-properties]
name = ty_rad_sw
type = ddt
[ccpp-arg-table]
name = ty_rad_sw
type = ddt
[ sfc_up_sw ]
standard_name = surface_upwelling_shortwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys
[ sfc_down_sw ]
standard_name = surface_downwelling_shortwave_radiation_flux
units = W m2
dimensions = ()
type = real
kind = kind_phys
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_lw.F90
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module rad_lw
use ccpp_kinds, only: kind_phys
use mod_rad_ddt, only: ty_rad_lw

implicit none
private

public :: rad_lw_run

contains

!> \section arg_table_rad_lw_run Argument Table
!! \htmlinclude arg_table_rad_lw_run.html
!!
subroutine rad_lw_run(ncol, fluxLW, errmsg, errflg)

integer, intent(in) :: ncol
type(ty_rad_lw), intent(inout) :: fluxLW(:)
character(len=512), intent(out) :: errmsg
integer, intent(out) :: errflg

! Locals
integer :: icol

errmsg = ''
errflg = 0

do icol=1,ncol
fluxLW(icol)%sfc_up_lw = 300._kind_phys
fluxLW(icol)%sfc_down_lw = 50._kind_phys
enddo

end subroutine rad_lw_run

end module rad_lw
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_lw.meta
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
[ccpp-table-properties]
name = rad_lw
type = scheme
dependencies = module_rad_ddt.F90
[ccpp-arg-table]
name = rad_lw_run
type = scheme
[ ncol ]
standard_name = horizontal_loop_extent
type = integer
units = count
dimensions = ()
intent = in
[fluxLW]
standard_name = longwave_radiation_fluxes
long_name = longwave radiation fluxes
units = W m-2
dimensions = (horizontal_loop_extent)
type = ty_rad_lw
intent = inout
[ errmsg ]
standard_name = ccpp_error_message
long_name = Error message for error handling in CCPP
units = none
dimensions = ()
type = character
kind = len=512
intent = out
[ errflg ]
standard_name = ccpp_error_code
long_name = Error flag for error handling in CCPP
units = 1
dimensions = ()
type = integer
intent = out
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_sw.F90
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module rad_sw
use ccpp_kinds, only: kind_phys
use mod_rad_ddt, only: ty_rad_sw

implicit none
private

public :: rad_sw_run

contains

!> \section arg_table_rad_sw_run Argument Table
!! \htmlinclude arg_table_rad_sw_run.html
!!
subroutine rad_sw_run(ncol, fluxSW, errmsg, errflg)

integer, intent(in) :: ncol
type(ty_rad_sw), intent(inout) :: fluxSW(:)
character(len=512), intent(out) :: errmsg
integer, intent(out) :: errflg

! Locals
integer :: icol

errmsg = ''
errflg = 0

do icol=1,ncol
fluxSW(icol)%sfc_up_sw = 100._kind_phys
fluxSW(icol)%sfc_down_sw = 400._kind_phys
enddo

end subroutine rad_sw_run

end module rad_sw
35 changes: 35 additions & 0 deletions test/var_compatibility_test/rad_sw.meta
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
[ccpp-table-properties]
name = rad_sw
type = scheme
dependencies = module_rad_ddt.F90
[ccpp-arg-table]
name = rad_sw_run
type = scheme
[ ncol ]
standard_name = horizontal_loop_extent
type = integer
units = count
dimensions = ()
intent = in
[fluxSW]
standard_name = shortwave_radiation_fluxes
long_name = shortwave radiation fluxes
units = W m-2
dimensions = (horizontal_loop_extent)
type = ty_rad_sw
intent = inout
[ errmsg ]
standard_name = ccpp_error_message
long_name = Error message for error handling in CCPP
units = none
dimensions = ()
type = character
kind = len=512
intent = out
[ errflg ]
standard_name = ccpp_error_code
long_name = Error flag for error handling in CCPP
units = 1
dimensions = ()
type = integer
intent = out
16 changes: 11 additions & 5 deletions test/var_compatibility_test/run_test
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ ccpp_files="${utility_files}"
ccpp_files="${ccpp_files},${build_dir}/ccpp/test_host_ccpp_cap.F90"
ccpp_files="${ccpp_files},${build_dir}/ccpp/ccpp_var_compatibility_suite_cap.F90"
#process_list=""
module_list="effr_calc,effr_diag,effr_post,effr_pre"
#dependencies=""
module_list="effr_calc,effr_diag,effr_post,effr_pre,rad_lw,rad_sw"
dependencies="module_rad_ddt"
suite_list="var_compatibility_suite"
required_vars_var_compatibility="ccpp_error_code,ccpp_error_message"
required_vars_var_compatibility="${required_vars_var_compatibility},cloud_graupel_number_concentration"
Expand All @@ -143,8 +143,10 @@ required_vars_var_compatibility="${required_vars_var_compatibility},flag_indicat
required_vars_var_compatibility="${required_vars_var_compatibility},horizontal_dimension"
required_vars_var_compatibility="${required_vars_var_compatibility},horizontal_loop_begin"
required_vars_var_compatibility="${required_vars_var_compatibility},horizontal_loop_end"
required_vars_var_compatibility="${required_vars_var_compatibility},longwave_radiation_fluxes"
required_vars_var_compatibility="${required_vars_var_compatibility},scalar_variable_for_testing"
required_vars_var_compatibility="${required_vars_var_compatibility},scheme_order_in_suite"
required_vars_var_compatibility="${required_vars_var_compatibility},scheme_order_in_suite"
required_vars_var_compatibility="${required_vars_var_compatibility},shortwave_radiation_fluxes"
required_vars_var_compatibility="${required_vars_var_compatibility},vertical_layer_dimension"
input_vars_var_compatibility="cloud_graupel_number_concentration"
#input_vars_var_compatibility="${input_vars_var_compatibility},cloud_ice_number_concentration"
Expand All @@ -157,17 +159,21 @@ input_vars_var_compatibility="${input_vars_var_compatibility},flag_indicating_cl
input_vars_var_compatibility="${input_vars_var_compatibility},horizontal_dimension"
input_vars_var_compatibility="${input_vars_var_compatibility},horizontal_loop_begin"
input_vars_var_compatibility="${input_vars_var_compatibility},horizontal_loop_end"
input_vars_var_compatibility="${input_vars_var_compatibility},longwave_radiation_fluxes"
input_vars_var_compatibility="${input_vars_var_compatibility},scalar_variable_for_testing"
input_vars_var_compatibility="${input_vars_var_compatibility},scheme_order_in_suite"
input_vars_var_compatibility="${input_vars_var_compatibility},shortwave_radiation_fluxes"
input_vars_var_compatibility="${input_vars_var_compatibility},vertical_layer_dimension"
output_vars_var_compatibility="ccpp_error_code,ccpp_error_message"
output_vars_var_compatibility="${output_vars_var_compatibility},cloud_ice_number_concentration"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_ice_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_liquid_water_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_rain_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},effective_radius_of_stratiform_cloud_snow_particle"
output_vars_var_compatibility="${output_vars_var_compatibility},longwave_radiation_fluxes"
output_vars_var_compatibility="${output_vars_var_compatibility},scalar_variable_for_testing"
output_vars_var_compatibility="${output_vars_var_compatibility},scheme_order_in_suite"
output_vars_var_compatibility="${output_vars_var_compatibility},shortwave_radiation_fluxes"

##
## Run a database report and check the return string
Expand Down Expand Up @@ -233,8 +239,8 @@ echo -e "\nChecking lists from command line"
#check_datatable ${report_prog} ${datafile} "--process-list" ${process_list}
check_datatable ${report_prog} ${datafile} "--module-list" ${module_list}
#check_datatable ${report_prog} ${datafile} "--dependencies" ${dependencies}
check_datatable ${report_prog} ${datafile} "--suite-list" ${suite_list} \
--sep ";"
#check_datatable ${report_prog} ${datafile} "--suite-list" ${suite_list} \
# --sep ";"
echo -e "\nChecking variables for var_compatibility suite from command line"
check_datatable ${report_prog} ${datafile} "--required-variables" \
${required_vars_var_compatibility} "var_compatibility_suite"
Expand Down
Loading