Add libFMS module #705
Add libFMS module #705
Conversation
… to data_override
rem1776
requested review from
bensonr,
thomas-robinson and
wrongkindofdoctor
as code owners
libFMS_mod/libFMS.F90
Outdated
| use mpp_mod, only: mpp_init_test_full_init, mpp_init_test_init_true_only, & | ||
| mpp_init_test_peset_allocated, mpp_init_test_clocks_init, & | ||
| mpp_init_test_datatype_list_init, mpp_init_test_logfile_init, & | ||
| mpp_init_test_read_namelist, mpp_init_test_etc_unit, & | ||
| mpp_init_test_requests_allocated, stdin, stdout, stderr, & |
There was a problem hiding this comment.
I don't think we want to expose mpp_init_test_* to the public. Our unit tests can grab them directly from mpp_mod.
There was a problem hiding this comment.
Removed these in 056a29f
libFMS_mod/libFMS.F90
Outdated
| use sat_vapor_pres_k_mod, only: sat_vapor_pres_init_k, lookup_es_k, lookup_des_k, & | ||
| lookup_es_des_k, lookup_es2_k, lookup_des2_k, & | ||
| lookup_es2_des2_k, lookup_es3_k, lookup_des3_k, & | ||
| lookup_es3_des3_k, compute_qs_k, compute_mrs_k |
There was a problem hiding this comment.
These should only be used by sat_vapor_pres.F90 and don't need to be made public.
There was a problem hiding this comment.
Removed with 056a29f
libFMS_mod/libFMS.F90
Outdated
| interpolator_end, init_clim_diag, query_interpolator, & | ||
| interpolate_type, CONSTANT, & | ||
| INTERP_WEIGHTED_P, INTERP_LINEAR_P, INTERP_LOG_P, & | ||
| ZERO_INTERP=>ZERO, & !! conflicts with mpp_parameter's ZERO |
There was a problem hiding this comment.
The fact we have a parameter named ZERO in interpolator_mod that is given a value of '2' is a real danger. If the ZERO parameter from this module is not used in any of our actual model code, we should remove it's public specification and rename it internally to make it's use more clear.
There was a problem hiding this comment.
It looks an array of the ZERO or CONSTANT flags is used for calling interpolator_init, so I think it may need to stay public to use that routine.
libFMS_mod/libFMS.F90
Outdated
| use fm_util_mod, only: fm_util_start_namelist, fm_util_end_namelist, & | ||
| fm_util_check_for_bad_fields, fm_util_set_caller, & | ||
| fm_util_reset_caller, fm_util_set_no_overwrite, & | ||
| fm_util_reset_no_overwrite, fm_util_set_good_name_list, & | ||
| fm_util_reset_good_name_list, fm_util_get_length, & | ||
| fm_util_get_integer, fm_util_get_logical, fm_util_get_real, & | ||
| fm_util_get_string, fm_util_get_integer_array, & | ||
| fm_util_get_logical_array, fm_util_get_real_array, & | ||
| fm_util_get_string_array, fm_util_set_value, & | ||
| fm_util_set_value_integer_array, fm_util_set_value_logical_array, & | ||
| fm_util_set_value_real_array, fm_util_set_value_string_array, & | ||
| fm_util_set_value_integer, fm_util_set_value_logical, & | ||
| fm_util_set_value_real, fm_util_set_value_string, & | ||
| fm_util_get_index_list, fm_util_get_index_string, & | ||
| fm_util_default_caller |
There was a problem hiding this comment.
Need to understand if these are used externally or have a valid external use before exposing them to the public via this interface.
There was a problem hiding this comment.
These aren't used in the field_manager module, a few routines are used in coupler/atmos_ocean_fluxes but I think they are meant to be used externally since some aren't called within FMS.
libFMS_mod/libFMS.F90
Outdated
| use drifters_comm_mod, only: drifters_comm_Type, drifters_comm_new, & | ||
| drifters_comm_del, drifters_comm_set_pe_neighbors, & | ||
| drifters_comm_set_domain, drifters_comm_update, & | ||
| drifters_comm_gather |
There was a problem hiding this comment.
Need to understand if these are used externally or have a valid external use before exposing them to the public via this interface.
There was a problem hiding this comment.
Removed this with c267040, since it looks like it's only meant to be used in drifters_mod.
Should the other drifters_*_mod modules be removed as well?
Looking over it again it seems the drifters_*_types from the other modules are encapsulated within drifter_type so those other modules aren't necessary for using drifters_mod. The only issue is drifters_type has all those types public so they can still be accessed, although I'm not sure if that's intentional.
libFMS_mod/libFMS.F90
Outdated
| use diag_grid_mod, only: diag_grid_init, diag_grid_end, get_local_indexes, & | ||
| get_local_indexes2 | ||
| use diag_table_mod, only: parse_diag_table | ||
| use diag_util_mod, only: get_subfield_size, log_diag_field_info, update_bounds, & | ||
| check_out_of_bounds, check_bounds_are_exact_dynamic, & | ||
| check_bounds_are_exact_static, init_file, diag_time_inc, & | ||
| find_input_field, init_input_field, init_output_field, & | ||
| diag_data_out, write_static, check_duplicate_output_fields, & | ||
| get_date_dif, get_subfield_vert_size, sync_file_times, & | ||
| prepend_attribute, attribute_init, diag_util_init | ||
| use diag_output_mod, only: diag_output_init, write_axis_meta_data, write_field_meta_data, & | ||
| done_meta_data, get_diag_global_att, & | ||
| set_diag_global_att, diag_field_write, diag_write_time | ||
| use diag_axis_mod, only: diag_axis_init, get_diag_axis, get_domain1d, & | ||
| get_domain2d, get_axis_length, get_axis_global_length, & | ||
| diag_subaxes_init, get_diag_axis_cart, get_diag_axis_data, & | ||
| max_axes, get_axis_aux, get_tile_count, get_axes_shift, & | ||
| get_diag_axis_name, get_axis_num, get_diag_axis_domain_name, & | ||
| diag_axis_add_attribute, get_domainUG, axis_compatible_check, & | ||
| axis_is_compressed, get_compressed_axes_ids, get_axis_reqfld, & | ||
| DIAG_AXIS_NODOMAIN, DIAG_AXIS_2DDOMAIN, DIAG_AXIS_UGDOMAIN | ||
| use diag_data_mod, only: MAX_FIELDS_PER_FILE, DIAG_OTHER, DIAG_OCEAN, DIAG_ALL, & | ||
| VERY_LARGE_FILE_FREQ, VERY_LARGE_AXIS_LENGTH, EVERY_TIME, & | ||
| END_OF_RUN, DIAG_SECONDS, DIAG_MINUTES, DIAG_HOURS, DIAG_DAYS, & | ||
| DIAG_MONTHS, DIAG_YEARS, MAX_SUBAXES, GLO_REG_VAL, & | ||
| GLO_REG_VAL_ALT, CMOR_MISSING_VALUE, DIAG_FIELD_NOT_FOUND, & | ||
| num_files, num_input_fields, num_output_fields, null_axis_id, & | ||
| diag_grid, diag_fieldtype, diag_atttype, coord_type, file_type, & | ||
| input_field_type, output_field_type, diag_axis_type, & | ||
| diag_global_att_type, FILL_VALUE, EMPTY, & | ||
| MAX_VALUE, MIN_VALUE, diag_init_time, base_time, & | ||
| base_year, base_month, base_day, base_hour, base_minute, & | ||
| base_second, global_descriptor |
There was a problem hiding this comment.
Need to understand if these are used externally or have a valid external use before exposing them to the public via this interface.
There was a problem hiding this comment.
diag_axis_init is externally used. Not sure about the others.
libFMS_mod/libFMS.F90
Outdated
| !! get_grid_version_mpp_mod, mpp_io_mod, mosaic_mod, | ||
| !! fms_mod(partial, old io excluded) | ||
| !! | ||
| module fms |
There was a problem hiding this comment.
I thought this was going to be in the top level directory. Or am I remember that wrong?
There was a problem hiding this comment.
I'm not sure to be honest, but I moved it with c267040 since the two libFMS folders is a bit confusing anyway
libFMS_mod/libFMS.F90
Outdated
| use diag_grid_mod, only: diag_grid_init, diag_grid_end, get_local_indexes, & | ||
| get_local_indexes2 | ||
| use diag_table_mod, only: parse_diag_table | ||
| use diag_util_mod, only: get_subfield_size, log_diag_field_info, update_bounds, & | ||
| check_out_of_bounds, check_bounds_are_exact_dynamic, & | ||
| check_bounds_are_exact_static, init_file, diag_time_inc, & | ||
| find_input_field, init_input_field, init_output_field, & | ||
| diag_data_out, write_static, check_duplicate_output_fields, & | ||
| get_date_dif, get_subfield_vert_size, sync_file_times, & | ||
| prepend_attribute, attribute_init, diag_util_init | ||
| use diag_output_mod, only: diag_output_init, write_axis_meta_data, write_field_meta_data, & | ||
| done_meta_data, get_diag_global_att, & | ||
| set_diag_global_att, diag_field_write, diag_write_time | ||
| use diag_axis_mod, only: diag_axis_init, get_diag_axis, get_domain1d, & | ||
| get_domain2d, get_axis_length, get_axis_global_length, & | ||
| diag_subaxes_init, get_diag_axis_cart, get_diag_axis_data, & | ||
| max_axes, get_axis_aux, get_tile_count, get_axes_shift, & | ||
| get_diag_axis_name, get_axis_num, get_diag_axis_domain_name, & | ||
| diag_axis_add_attribute, get_domainUG, axis_compatible_check, & | ||
| axis_is_compressed, get_compressed_axes_ids, get_axis_reqfld, & | ||
| DIAG_AXIS_NODOMAIN, DIAG_AXIS_2DDOMAIN, DIAG_AXIS_UGDOMAIN | ||
| use diag_data_mod, only: MAX_FIELDS_PER_FILE, DIAG_OTHER, DIAG_OCEAN, DIAG_ALL, & | ||
| VERY_LARGE_FILE_FREQ, VERY_LARGE_AXIS_LENGTH, EVERY_TIME, & | ||
| END_OF_RUN, DIAG_SECONDS, DIAG_MINUTES, DIAG_HOURS, DIAG_DAYS, & | ||
| DIAG_MONTHS, DIAG_YEARS, MAX_SUBAXES, GLO_REG_VAL, & | ||
| GLO_REG_VAL_ALT, CMOR_MISSING_VALUE, DIAG_FIELD_NOT_FOUND, & | ||
| num_files, num_input_fields, num_output_fields, null_axis_id, & | ||
| diag_grid, diag_fieldtype, diag_atttype, coord_type, file_type, & | ||
| input_field_type, output_field_type, diag_axis_type, & | ||
| diag_global_att_type, FILL_VALUE, EMPTY, & | ||
| MAX_VALUE, MIN_VALUE, diag_init_time, base_time, & | ||
| base_year, base_month, base_day, base_hour, base_minute, & | ||
| base_second, global_descriptor |
There was a problem hiding this comment.
diag_axis_init is externally used. Not sure about the others.
libFMS_mod/libFMS.F90
Outdated
| !! Remappings due to conflicts: | ||
| !! get_mosaic_tile_grid from mosaic2(fms2_io) => get_mosaic_tile_grid_2 | ||
| !! read_data from interpolator_mod(fms2_io) => read_data_interp | ||
| !! ZERO from interpolator_mod(mpp_parameter) => ZERO_INTERP | ||
| !! | ||
| !! Not in this module: axis_utils_mod, fms_io_mod, time_interp_external_mod | ||
| !! get_grid_version_mpp_mod, mpp_io_mod, mosaic_mod, | ||
| !! fms_mod(partial, old io excluded) |
There was a problem hiding this comment.
Anything that will be deprecated with fms_io and mpp_io should be left out.
I think you should reverse the renaming convention if you are going to remap some of the routines by putting their origin at the beginning. interpolator_read_data instead of read_data_interp.
There was a problem hiding this comment.
renamed with that convention with c267040. The two renamed routines both use fms2_io, but also conflicted with it.
…emoved drifters_comm_mod
libFMS.F90
Outdated
| use diag_output_mod, only: diag_output_init, write_axis_meta_data, write_field_meta_data, & | ||
| done_meta_data, get_diag_global_att, & | ||
| set_diag_global_att, diag_field_write, diag_write_time | ||
| use diag_axis_mod, only: diag_axis_init, get_diag_axis, get_domain1d, & |
There was a problem hiding this comment.
You need to include diag_axis_init in the public routines
There was a problem hiding this comment.
Idk how that happened, must've forgot to add the file again before the commit. Fixed now
|
@rem1776 (and others), I have been thinking about this and our ultimate goal with this type of change is to hide the interfaces we don't want model components to use. We have had some developers begin to use interfaces we would rather them not use in ways we weren't expecting (reading in ascii data comes to mind). With what I think is the current approach, users will still have access to the other routines since the To get what we what, we will need to find a way to only expose the interfaces we want to the user, but still have access throughout the FMS code. I can come up with three ways to do this:
Option 2, in my opinion, is not the way to go. It would likely lead to extremely large Fortran files or another #include hell, similar to the current MPP. Both would make it difficult to maintain the code. Option 3, while viable, would require more effort as we would need to check if any routines perform better in C or Fortran. Option 1, again in my opinion, is likely the solution that will take the least effort as most of the code will not need to change. To better explain how option 1 would work, I have put together a simple case (included with the comment as an attachment). This example has a program that uses a Fortran module. The module contains two public interfaces and use a Fortran One con to using option 1, as one can see in the example, a change to an interface would likely require the interface definition to be updated in two files. While this is something Fortran devs have not had to deal with, C/C++ dev have since the beginning. @rem1776, please review the example, and let me know if you have any questions. If we decide to go this route, we can likely divide the work among all FMS developers, but that is something @jwdGFDL will need to decide. Comments and opionions from others would be appreciated. |
|
@underwoo The example looks like a good way to setup the modules without an excess of them; from working on this it seems like most directories in FMS use a sort of 'main' module, in which any needed routines from it or any of it's helper/utility modules are made public (like in fms2_io, diag_manager), but not always and the helper modules can still be used the same way, making it hard to distinguish. Would implementing 1 require these 'helper' modules to become files containing functions/routines like get_double.f90 in the example? Would that still work with any global variables a module might have? |
|
I agree that path 1. seems the least stressful from a programming point
of view and is consistent with the C/C++ norms. It can also be handled
incrementally.
On the other hand, another (or at least an interim) approach might be
provide a manifest of interfaces for public use. We should be providing
that type of documentation anyway and I don't think most developers
outside of f-group will go digging through code for interfaces we're not
telling them about. We could also consider providing a list of things
that should not be used.
The current situation that we're untangling is due to a number of
historic "features" that I won't detail here. But it may be that
improved documentation as well as explicitly discouraging words will be
enough to allow us to take a leisurely pace in retrofitting the existing
code base while new code such as the *_manager rewrites afford the
opportunity to see how this will work in practice.
j
…On 3/11/21 3:35 PM, Seth Underwood wrote:
@rem1776 <https://github.com/rem1776> (and others), I have been
thinking about this and our ultimate goal with this type of change is
to /hide/ the interfaces we don't want model components to use. We
have had some developers begin to use interfaces we would rather them
not use in ways we weren't expecting (reading in ascii data comes to
mind). With what I think is the current approach, users will still
have access to the other routines since the |*.mod| files will still
be generated. Even if we install the library, /all/ the |*.mod| files
will need to be installed. Some compilers (PGI) will need them as the
compiler traces its way through the used interfaces.
To get what we what, we will need to find a way to only expose the
interfaces we want to the user, but still have access throughout the
FMS code. I can come up with three ways to do this:
1. Move away from using Fortran modules except for the exposed interfaces
2. Create fewer Fortran modules, and then use private/public
3. Rewrite large portions of libFMS in C, and then write Fortran
routines that wrap the C routines
Option 2, in my opinion, is not the way to go. It would likely lead to
extremely large Fortran files or another #include hell, similar to the
current MPP. Both would make it difficult to maintain the code.
Option 3, while viable, would require more effort as we would need to
check if any routines perform better in C or Fortran.
This might be a project, but I don't think we have the man hours
currently to take on a project like this.
Option 1, again in my opinion, is likely the solution that will take
the least effort as most of the code will not need to change. To
better explain how option 1 would work, I have put together a simple
case (included with the comment as an attachment
<https://github.com/NOAA-GFDL/FMS/files/6125805/no-f90-mod.tar.gz>).
This example has a program that uses a Fortran module. The module
contains two public interfaces and use a Fortran |include| statement
to include the interface information for a function that is in a third
file. The included |Makefile| will build the test.
One con to using option 1, as one can see in the example, a change to
an interface would likely require the interface definition to be
updated in two files. While this is something Fortran devs have not
had to deal with, C/C++ dev have since the beginning.
@rem1776 <https://github.com/rem1776>, please review the example, and
let me know if you have any questions. If we decide to go this route,
we can likely divide the work among all FMS developers, but that is
something @jwdGFDL <https://github.com/jwdGFDL> will need to decide.
Comments and opionions from others would be appreciated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#705 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABXB2ACRYVWHRJZLROOE4LTTDELRTANCNFSM4Y4MVS4Q>.
--
Jeff Durachta, PhD
Pronouns: he/him/his
Engineering Lead for Modeling Systems
NOAA Geophysical Fluid Dynamics Lab
Forrestal Campus, Princeton University
201 Forrestal Road
Princeton, NJ 08540
Office: +1-609-987-5054
|
…sary imports from libFMS.F90
|
I've added a markdown file to list the available supported interfaces/routines through the fms module and included any public types as well. I also updated the doxygen comments to make note of this. I added in any new routines and removed a few imports as well after looking it over again since they were used by other interfaces and didn't seem necessary to make public (get_grid_version from data_override, some subroutines in field manager that were overloaded, mersenne_twister_mod) I also noticed a typo in the original code for mpp_efp and renamed the mis-spelt routine(mpp_reset_efp_overlow_error). |
Description
Adds a module to provide access to all supported routines/values/functions via
use fms. Imports everything explicitly to avoid any unwanted/deprecated imports. Two routines and one constant were renamed due to namespace conflicts, and are noted in the description of libFMS.F90. (also adds some missing licenses in /data_override)How Has This Been Tested?
Tested via the unit tests by replacing any supported modules with this module.
Checklist:
make distcheckpasses