Mixed precision `data_override_mod` #1323

J-Lentz · 2023-08-09T17:22:27Z

Mixed precision support has been added to data_override_mod. It has been split into separate implementation and wrapper modules.

How Has This Been Tested?
Github CI and unit tests

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included
make distcheck passes

`data_override_mod` has been split into separate implementation and wrapper modules. Mixed precision support has been added.

mlee03 · 2023-08-11T14:19:38Z

data_override/data_override_impl.F90

@@ -0,0 +1,2 @@
+#include "data_override_impl_r4.fh"


is there a need for this extra file? It seems to slightly overcomplicate things

Removed in 28b3d59. Now the implementation modules are included at the top of data_override.F90 instead.

mlee03 · 2023-08-11T14:20:19Z

data_override/include/data_override_impl.inc

-use fms2_io_mod,     only : FmsNetcdfFile_t, open_file, close_file, &
-                            read_data, fms2_io_init, variable_exists, &
-                            get_mosaic_tile_file
+module DATA_OVERRIDE_IMPL_


Is there a need for the "IMPL"?

As opposed to just calling the macro DATA_OVERRIDE_?

a66917f: _IMPL has been removed from the macro names for DATA_OVERRIDE_0D_, DATA_OVERRIDE_2D_, DATA_OVERRIDE_3D_, DATA_OVERRIDE_UG_1D_, and DATA_OVERRIDE_UG_2D_.

I've left DATA_OVERRIDE_INIT_IMPL_ alone to avoid confusion with the data_override_init wrapper subroutine.

thomas-robinson

Documentation needs to be updated, especially on new routines and new variables.
I'm not sure why there are routines that are used to select calling a r4 or r8 routine instead of using an interface.
(or 2.5) My understanding was that unlimited polymorphism wasn't going to be used and instead we were using macros. This PR uses both. Please work with @mlee03 to decide how to proceed. If you do use class(*) because it is needed/neater or whatever, please make sure you document in the code why that was done.

thomas-robinson · 2023-08-14T10:40:53Z

data_override/data_override.F90

+integer :: atm_mode = 0
+integer :: ocn_mode = 0
+integer :: lnd_mode = 0
+integer :: ice_mode = 0


These are new variables. They need to be documented with doxygen comments.

Why are they all 0?

I added the class(*) dispatcher subroutines because I didn't realize that module procedure x could be used when x is a use'd subroutine (I thought module procedure was only for procedures in the module's contains section). But it actually seems to work fine, so the class(*) subroutines have been replaced by interfaces in b89c8db.

atm_mode, ocn_mode, lnd_mode, and ice_mode all take on values 0 (uninitialized), 4 (r4 mode), and 8 (r8 mode). This will be clarified with doxygen comments.

Comments added in eb0137c.

thomas-robinson · 2023-08-14T10:49:03Z

data_override/data_override.F90

@@ -182,381 +93,35 @@ end function count_ne_1
 !! Data_table is initialized here with default values. Users should provide "real" values
 !! that will override the default values. Real values can be given using data_table, each
 !! line of data_table contains one data_entry. Items of data_entry are comma separated.
-subroutine data_override_init(Atm_domain_in, Ocean_domain_in, Ice_domain_in, Land_domain_in, Land_domainUG_in)
+subroutine data_override_init(Atm_domain_in, Ocean_domain_in, Ice_domain_in, Land_domain_in, Land_domainUG_in, mode)


You need to update the description of this routine, because it looks like all it's doing is calling another routine and setting up the "mode" which is not clearly defined

Updated in e960b14.

thomas-robinson · 2023-08-14T10:49:45Z

data_override/data_override.F90

+  integer, intent(in), optional :: mode !< r4_kind or r8_kind
+  integer :: mode_selector


These variables need better documentation. What is mode? The mode of what?

Meatier comment added in eb0137c.

thomas-robinson · 2023-08-14T10:50:35Z

data_override/data_override.F90

+  if (present(Atm_domain_in))   atm_mode = mode
+  if (present(Ocean_domain_in)) ocn_mode = mode
+  if (present(Ice_domain_in))   ice_mode = mode
+  if (present(Land_domain_in))  lnd_mode = mode


This is not going to work.

mode is an optional argument, so it should be wrapped in an if (present(mode)) or am i missing something?

The different components do not need to be the same type. If the ocean is running with r4 and the atmosphere in r8, how is this going to support mixed-more? It's not clear to me what this is doing, so it needs better documentation.

Just a careless mistake, fixed in 5dacac8

This would be accomplished with multiple calls to data_override_init, e.g.:

call data_override_init(ocean_domain_in=..., mode=r4_kind) call data_override_init(atm_domain_in=..., mode=r8_kind)

The doxygen comment does currently state that data_override_init can be called multiple times. I'll add a description of the behavior of the new mode argument.

thomas-robinson · 2023-08-14T10:52:00Z

data_override/data_override.F90

@@ -569,120 +134,93 @@ subroutine data_override_unset_domains(unset_Atm, unset_Ocean, &

  fail_if_not_set = .true. ; if (present(must_be_set)) fail_if_not_set = must_be_set

-  if (.not.module_is_initialized) call mpp_error(FATAL, &


Why remove the error check?

module_is_initialized now lives in the r4 and r8 versions of the implementation modules, rather than in the wrapper module.

The new atm_mode, ocn_mode, lnd_mode, and ice_mode variables now keep track of what has and hasn't been initialized, so mpp_error still gets called if the user tries to unset an uninitialized domain.

thomas-robinson · 2023-08-14T12:14:14Z

data_override/include/data_override_impl.inc

-!! that will override the default values. Real values can be given using data_table, each
-!! line of data_table contains one data_entry. Items of data_entry are comma separated.
-subroutine data_override_init(Atm_domain_in, Ocean_domain_in, Ice_domain_in, Land_domain_in, Land_domainUG_in)
+subroutine DATA_OVERRIDE_INIT_IMPL_(Atm_domain_in, Ocean_domain_in, Ice_domain_in, Land_domain_in, Land_domainUG_in)


This routine needs documentation

Doxygen comment added in 2d6bfd6.

thomas-robinson · 2023-08-14T12:15:07Z

data_override/include/data_override_impl.inc

@@ -351,8 +274,14 @@ endif
 if(use_get_grid_version .EQ. 2) then
   call close_file(fileobj)
 end if
+end subroutine DATA_OVERRIDE_INIT_IMPL_
+
+function count_ne_1(in_1, in_2, in_3)


This function needs documentation

Doxygen comment added in 59bf15d.

thomas-robinson · 2023-08-14T12:15:22Z

data_override/include/data_override_impl.inc

+  logical, intent(in)  :: in_1, in_2, in_3
+  logical :: count_ne_1


these variables need documentation

Added in 59bf15d.

thomas-robinson · 2023-08-14T12:17:28Z

data_override/include/data_override_impl.inc

+  if(PRESENT(override)) override = .true.
+
+end subroutine DATA_OVERRIDE_0D_IMPL_
+
 !> @brief This routine performs data override for 2D fields; for usage, see data_override_3d.


I think you want to see data_override_3d_impl_ . Please update this comment.

I've removed "for usage, see data_override_3d" in e960b14.

thomas-robinson · 2023-08-14T12:23:13Z

data_override/include/data_override_impl.inc

-end module data_override_mod
-!> @}
-! close documentation grouping
+end module


Please include the module name here

Include files for the implementation modules at the top of data_override.F90, and remove data_override_impl.F90

Replace the class(*) versions of the `data_override_*` procedures with interfaces.

Fix `data_override_init` in the case where the optional `mode` argument is not used.

`DATA_OVERRIDE_0D_IMPL_` -> `DATA_OVERRIDE_0D_` `DATA_OVERRIDE_2D_IMPL_` -> `DATA_OVERRIDE_2D_` `DATA_OVERRIDE_3D_IMPL_` -> `DATA_OVERRIDE_3D_` `DATA_OVERRIDE_UG_1D_IMPL_` -> `DATA_OVERRIDE_UG_1D_` `DATA_OVERRIDE_UG_2D_IMPL_` -> `DATA_OVERRIDE_UG_2D_`

mlee03 · 2023-08-25T16:41:48Z

cool :) For consistency, could you change DATA_OVERRIDE_KIND_ to start with FMS_DATA_OVERRIDE_KIND_ (or something like that that starts with FMS). Same for GET_GRID_VERSION_KIND_

J-Lentz · 2023-08-25T17:27:53Z

Macro names changed in 2b71679.

rem1776

I don't think we combine multiple modules in the same file elsewhere in fms, do you know if it gives the right line numbers in tracebacks for includes if you got an error from one of those r4/r8 routines?

Creating two more files just to include the two kind versions doesn't make much more sense either though, so I would add some comments since this switches up the typical mixed precision CPP method. A line or two in the header comments for data_override.F90 and data_override.inc explaining how this include is used as a module in this case specifically would be good.

data_override/Makefile.am

data_override/data_override.F90

mlee03 · 2023-08-29T14:53:20Z

@rem1776 @J-Lentz, I can see how using FMS_DATA_OVERRIDE_TYPE and explicitly declaring two versions of the private types (as done in the rest of mixed-precision FMS) could work, but I like how data_override is modified here, it's very clean. And if we in the future add submodules, the changes would be straightforward here. And agree with @rem1776, I now see how the mixed precision implemented here is not the easiest to understand and a bit of comment would be beneficial.

Comments have been added to the tops of `data_override.F90` and `data_override.inc`. The `module` and `module end` statements have been moved from the .fh files to `data_override.F90`.

J-Lentz · 2023-08-30T14:16:25Z

I've added brief comments to the tops of data_override.F90 and data_override.inc. And I've moved the module data_override_r* and end module data_override_r* statements to data_override.F90, because Fortran code in .fh files could be confusing and unexpected.

mlee03 · 2023-08-30T14:28:31Z

data_override/get_grid_version.F90

@@ -33,7 +34,16 @@ module get_grid_version_mod

 implicit none

-real, parameter    :: deg_to_radian=PI/180.


is it a private variable?

It was, but I've replaced it with DEG_TO_RAD from constants_mod.

mlee03 · 2023-08-30T14:32:12Z

data_override/include/data_override.inc

@@ -871,8 +907,8 @@ subroutine data_override_3d(gridname,fieldname_code,data,time,override,data_inde
           call mpp_error(FATAL,'data_override: file '//trim(filename)//' is not opened in time_interp_external')
        end if
 ! convert lon_in and lat_in from deg to radian
-        override_array(curr_position)%lon_in = override_array(curr_position)%lon_in * deg_to_radian
-        override_array(curr_position)%lat_in = override_array(curr_position)%lat_in * deg_to_radian
+        override_array(curr_position)%lon_in = override_array(curr_position)%lon_in * DEG_TO_RAD


real(DEG_TO_RAD,lkind)

Changed in da7da2c.

mlee03 · 2023-08-30T14:35:58Z

test_fms/data_override/test_get_grid_v1.F90

@@ -129,11 +131,11 @@ program test_get_grid_v1
                        min_lon, max_lon)

 !< Error checking:
-if (lon(1,1) .ne. sum(lon_vert_in)/4*deg_to_radian ) then
+if (lon(1,1) .ne. sum(lon_vert_in)/4*real(DEG_TO_RAD, lkind) ) then


Changed in da7da2c.

BREAKING CHANGE: In coupler_types.F90, `coupler_nd_field_type` and `coupler_nd_values_type` have been renamed to indicate real kind value: `coupler_nd_real4/8_field_type` and `coupler_nd_real4/8_values_type`. The `bc` field within `coupler_nd_bc_type` was modified to use r8_kind within the value and field types, and an additional field added `bc_r4` to use r4_kind values. Includes: * feat: eliminate use of real numbers for mixed precision in `block_control` (#1195) * feat: mixed precision field_manager (#1205) * feat: mixed precision random_numbers_mod (#1191) * feat: mixed precision time_manager reals to r8 and clean up (#1196) * feat: mixed Precision tracer_manager (#1212) * Mixed precision monin_obukhov (#1116) * Mixed precision: `monin_obukhov` unit tests (#1272) * mixed-precision diag_integral_mod (#1217) * mixed precision time_interp (#1252) * mixed precision interpolator_mod (#1305) * Mixed precision astronomy (#1092) * Mixed precision `data_override_mod` (#1323) * mixed precision exchange (#1341) * coupler mixed precision (#1353) * Mixed precision topography_mod (#1250) --------- Co-authored-by: rem1776 <[email protected]> Co-authored-by: MiKyung Lee <[email protected]> Co-authored-by: mlee03 <[email protected]> Co-authored-by: Caitlyn McAllister <[email protected]> Co-authored-by: Jesse Lentz <[email protected]>

Mixed precision data_override

89b76c8

`data_override_mod` has been split into separate implementation and wrapper modules. Mixed precision support has been added.

mlee03 reviewed Aug 11, 2023

View reviewed changes

thomas-robinson requested changes Aug 14, 2023

View reviewed changes

Jesse Lentz added 11 commits August 21, 2023 13:05

Remove data_override_impl.F90

28b3d59

Include files for the implementation modules at the top of data_override.F90, and remove data_override_impl.F90

Use interfaces instead of class(*) procedures

b89c8db

Replace the class(*) versions of the `data_override_*` procedures with interfaces.

Fix data_override_init

5dacac8

Fix `data_override_init` in the case where the optional `mode` argument is not used.

end module DATA_OVERRIDE_IMPL_

2b4b16e

Add doxygen comments to data_override.F90

eb0137c

Update and clarify doxygen comments

e960b14

Add doxygen comment for DATA_OVERRIDE_INIT_IMPL_

2d6bfd6

Fix r4_kind unit tests

3ca0798

Split long lines

3739e01

Merge remote-tracking branch 'NOAA-GFDL/mixedmode' into data_override

969920d

Add data_override/include to CMakeLists.txt

bbf8b19

J-Lentz marked this pull request as ready for review August 22, 2023 23:23

J-Lentz requested a review from rem1776 as a code owner August 22, 2023 23:23

J-Lentz requested a review from mcallic2 August 22, 2023 23:24

Jesse Lentz added 9 commits August 23, 2023 12:13

Add data_override/include to CMakeLists.txt

de4a15d

Rename data_override include files

db69da2

Document DATA_OVERRIDE_INIT_IMPL_ arguments

2478bf6

Revise Makefile.am

303141e

Add doxygen comments for count_ne_1

59bf15d

Merge remote-tracking branch 'NOAA-GFDL/mixedmode' into data_override

1f51673

Add GPL header to include files

fba5123

Attempt to fix CMake build

bfcba86

Remove _IMPL from macro names

a66917f

`DATA_OVERRIDE_0D_IMPL_` -> `DATA_OVERRIDE_0D_` `DATA_OVERRIDE_2D_IMPL_` -> `DATA_OVERRIDE_2D_` `DATA_OVERRIDE_3D_IMPL_` -> `DATA_OVERRIDE_3D_` `DATA_OVERRIDE_UG_1D_IMPL_` -> `DATA_OVERRIDE_UG_1D_` `DATA_OVERRIDE_UG_2D_IMPL_` -> `DATA_OVERRIDE_UG_2D_`

Change kind macro names

2b71679

mlee03 approved these changes Aug 28, 2023

View reviewed changes

rem1776 requested changes Aug 28, 2023

View reviewed changes

data_override/Makefile.am Outdated Show resolved Hide resolved

data_override/data_override.F90 Show resolved Hide resolved

Remove duplicate line in Makefile.am

ca0d5ab

mcallic2 approved these changes Aug 29, 2023

View reviewed changes

Add comments and move module/module end statements

d93c3cd

Comments have been added to the tops of `data_override.F90` and `data_override.inc`. The `module` and `module end` statements have been moved from the .fh files to `data_override.F90`.

mlee03 reviewed Aug 30, 2023

View reviewed changes

mlee03 suggested changes Aug 30, 2023

View reviewed changes

rem1776 approved these changes Aug 30, 2023

View reviewed changes

Eliminate implicit kinds and typecasts

da7da2c

mlee03 approved these changes Aug 30, 2023

View reviewed changes

rem1776 merged commit dffe74b into NOAA-GFDL:mixedmode Aug 30, 2023
19 checks passed

		integer, intent(in), optional :: mode !< r4_kind or r8_kind
		integer :: mode_selector

		@@ -569,120 +134,93 @@ subroutine data_override_unset_domains(unset_Atm, unset_Ocean, &

		fail_if_not_set = .true. ; if (present(must_be_set)) fail_if_not_set = must_be_set

		if (.not.module_is_initialized) call mpp_error(FATAL, &

		logical, intent(in) :: in_1, in_2, in_3
		logical :: count_ne_1

		@@ -33,7 +34,16 @@ module get_grid_version_mod

		implicit none

		real, parameter :: deg_to_radian=PI/180.

Mixed precision data_override_mod #1323

Mixed precision data_override_mod #1323

Conversation

J-Lentz commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-robinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlee03 commented Aug 25, 2023

J-Lentz commented Aug 25, 2023

rem1776 left a comment

Choose a reason for hiding this comment

mlee03 commented Aug 29, 2023

J-Lentz commented Aug 30, 2023 • edited Loading

mlee03 Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mixed precision `data_override_mod` #1323

Mixed precision `data_override_mod` #1323

J-Lentz commented Aug 30, 2023 •

edited

Loading

mlee03 Aug 30, 2023 •

edited

Loading