coupler mixed precision #1353

rem1776 · 2023-08-23T21:29:17Z

Description
Adds mixed precision support for the coupler subdirectory.

Majority of the changes are in coupler_types since it uses a few encapsulated types for 1-3 dimensional fields. The atmos_ocean_fluxes module has changes when needed to interact with the right type but does not take any required real arguments. Ensemble manager does not use reals (or the rest of this subdirectory) at all, so no changes have been made.

I noticed that in the FMScoupler only the coupler_Nd_bc_type is used so the r4/r8 split happens within it via the bc and bc_r4 members. This should be less disruptive for other codes using this type since theres nothing being removed for the most part, just the r4 stuff being added and used if needed.

Because the r4/r8 divide happens within the type, most routines that don't take reals just check which type has been associated (bc or bc_r4) and then use whats appropriate.

Fixes # (issue)

How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note
any relevant details for your test configuration (e.g. compiler, OS). Include
enough information so someone can reproduce your 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

…notest

coupler/coupler_types.F90

mlee03 · 2023-08-25T15:43:39Z

coupler/atmos_ocean_fluxes.F90

    logical, intent(in), dimension(:), optional :: flag !< flag
-    real, intent(in), optional                  :: mol_wt !< mol_wt
+    real(r8_kind), intent(in), optional                  :: mol_wt !< mol_wt


Do you think it'd be better if mol_wt was a class(*) here?

Here's where it ends up being used in this routine:

FMS/coupler/atmos_ocean_fluxes.F90

Lines 298 to 302 in 5d8e7ff

if (present(mol_wt)) then

call fm_util_set_value('mol_wt', mol_wt)

else

call fm_util_set_value('mol_wt', 0.0)

endif

But then also again here:

FMS/coupler/atmos_ocean_fluxes.F90

Line 645 in 5d8e7ff

gas_fluxes%bc(n)%mol_wt = fm_util_get_real('mol_wt', scalar = .true.)

I think fm_util_get_real is r8_kind only right cause of the read in strings?

that and because there was no way to tell what precision the user wanted based on the input arguments. I'm wondering if the users will pass in r4_kind mol_wt here because they're assuming that the r4 version of bc will be populated

Yeah to be safe I'll switch it over to a class(*), since it's intent(in) should be fine.

mlee03 · 2023-08-25T15:49:03Z

coupler/atmos_ocean_fluxes.F90

+        gas_fluxes%bc_r4(n)%atm_tr_index = fm_util_get_integer('atm_tr_index', scalar = .true.)
+
+        ! Save the molecular weight.
+        gas_fluxes%bc_r4(n)%mol_wt = fm_util_get_real('mol_wt', scalar = .true.)


should there be a real,r4_kind) around fm_util_get_real? I think fm_util_get_real returns an r8_kind

I just left the mol_wt and param fields as r8_kind regardless, this way they are always the value returned by the field manager.

mlee03 · 2023-08-25T18:05:51Z

coupler/atmos_ocean_fluxes.F90

-    allocate (gas_fluxes%bc(gas_fluxes%num_bcs))
-    allocate (gas_fields_atm%bc(gas_fields_atm%num_bcs))
-    allocate (gas_fields_ice%bc(gas_fields_ice%num_bcs))
+    if(present(use_r4_kind)) use_r4_kind_loc = use_r4_kind


wondering if this should go on top before the while loop

Not sure what you mean, I have this if for allocation here and then the while loop below has another if within it for the parts that use reals.

mlee03 · 2023-08-25T18:06:47Z

coupler/atmos_ocean_fluxes.F90

@@ -424,7 +424,7 @@ end function aof_set_coupler_flux
  !! @throw FATAL, "Num_flags is negative for [name]: [num_flags]"
  !! @throw FATAL, "Problem dumping fluxes tracer tree"
  !! @throw FATAL, "Number of fluxes does not match across the processors: [gas_fluxes%num_bcs] fluxes"
-  subroutine atmos_ocean_fluxes_init(gas_fluxes, gas_fields_atm, gas_fields_ice, verbosity)
+  subroutine atmos_ocean_fluxes_init(gas_fluxes, gas_fields_atm, gas_fields_ice, verbosity, use_r4_kind)


do you think there will be a case where the user wants these three fluxes to be set at different precisions?

I do not know honestly. It could be added using similar logic later on if we need it.

mlee03 · 2023-08-25T18:34:30Z

coupler/coupler_types.F90

@@ -412,6 +562,9 @@ subroutine coupler_type_copy_1d_2d(var_in, var_out, is, ie, js, je,&
      call mpp_error(FATAL, trim(error_header) // ' Number of output fields exceeds zero')
    endif

+    if(associated(var_in%bc) .eq. associated(var_in%bc_r4)) &


could there be a chance both are unassociated?

yeah definitely, thats why its comparing the logical return values. If they are both associated or both not associated it will error out, this way it'll ensure that the r4/r8 field types used are mutually exclusive.

In the code, the bc's could intentionally be not associated

the previous code would seg fault if you did that, these routines all essentially assume the child type is previously allocated for intent(in) types. edit: i was wrong, its only allocated when num_bc > 0. the loops that use bc/bc_r4 just don't run otherwise.

I put in a fix, the check for allocation needed to be after the checks for the optional as_needed argument. I also made updates so that it'll only check for one kind allocated if var%set is true since it indicates initialization.

…er_mp_notest

mlee03 · 2023-08-28T19:38:16Z

coupler/coupler_types.F90

-            & var%bc(n)%field(m)%name, axes(1:3), Time,&
-            & var%bc(n)%field(m)%long_name, var%bc(n)%field(m)%units )
+    if(var%set) then
+      if(associated(var%bc) .eqv. associated(var%bc_r4)) &


i think you missed one lol

should be all done now

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

coupler/coupler_types.F90

@@ -625,6 +783,12 @@ subroutine CT_spawn_1d_2d(var_in, var, idim, jdim, suffix, as_needed)
    if (.not.var_in%set)&
        & call mpp_error(FATAL, trim(error_header) // ' The parent type has not been initialized.')

+    ! check only one kind is used
+    if(var_in%set .and. var_in%num_bcs .gt. 0) then


is the var_in%set needed here since it's checked above? (not that it significantly matters)

nope (at least for the spawn routines), took these out with 27c6e87

mlee03 · 2023-08-29T16:59:58Z

@rem1776 could you add @mcallic2 and @J-Lentz as reviewers?

mlee03 · 2023-08-30T11:41:44Z

coupler/coupler_types.F90

-            do i=var%isc-halo,var%iec+halo
-              var%bc(n)%field(m)%values(i,j) = var_in%bc(n)%field(m)%values(i+i_off,j+j_off)
+    ! num_bcs .lt. 1 -> loop doesn't run but shouldn't error out
+    if (associated(var_in%bc) .or. var_in%num_bcs .lt. 1) then


Would it be clearer if var_in%num_bcs .lt. n1?

Also wondering if var_in%num_bcs.lt.1 should be separated into its own if else block for clarity

i think this is a little more clear. Some code within these is outside of loops so needs to run if either part of the conditional is true, otherwise will have to be repeated. could change the logic to use an .and. instead, but that would be more confusing

J-Lentz · 2023-08-30T15:31:55Z

axis_utils/include/axis_utils2.inc

Hmm, does this need to be merged with the current mixedmode branch?

should be updated, i left that one line deletion in axis utils cause that extra line will mess with the documentation parser.

mlee03 · 2023-08-30T18:18:57Z

CMakeLists.txt

@@ -319,6 +319,7 @@ foreach(kind ${kinds})
                                                 time_interp/include
                                                 tracer_manager/include
                                                 interpolator/include
+                                                 coupler/include


is there one more place to add coupler/include?

mlee03 · 2023-08-30T21:52:18Z

YAY! 🥳

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]>

mlee03 and others added 20 commits March 8, 2023 11:15

Update mixedmode_base branch to main (NOAA-GFDL#1144)

46d8e44

update the mixedmode_base branch to main (NOAA-GFDL#1154)

dde0b88

Merge branch 'mixedmode' into mixedmode_base

bc9e4d9

rm accidental empty line

2977578

chore: bring mixedmode_base up to date with main (NOAA-GFDL#1173)

b3374f6

chore: update mixedmode_base to main (NOAA-GFDL#1234)

cca0c7d

chore: merge main updates into mixedmode_base(NOAA-GFDL#1267)

712330b

mixed-precision: update mixedmode base (NOAA-GFDL#1289)

8c72630

readd file

19ee83f

working compile with type changes

1a309f4

finish up ifs for association

2c0b45c

update existing tests for r4 precision, fix missed bc_types

7d23cb8

add in test changes from other branch

baf95ca

add in mikyung's test

f693a46

more tests

f6503d7

mixed precision merge main into mixedmode_base (NOAA-GFDL#1342)

9517433

remove added test for pr

e27ec4a

Merge remote-tracking branch 'origin/mixedmode_base' into coupler_mp_…

49b3693

…notest

update mikyungs ocean fluxes test for both kinds

8c9eabf

missed r4 argument + macros for aof test

9c543de

rem1776 requested review from thomas-robinson and bensonr as code owners August 23, 2023 21:29

rem1776 and others added 2 commits August 24, 2023 09:52

Add in checks for only one kind associated

24933d7

Merge branch 'mixedmode' into coupler_mp_notest

5d8e7ff

mlee03 reviewed Aug 25, 2023

View reviewed changes

rem1776 added 3 commits August 25, 2023 15:31

Review comments + any lingering conversion warnings

1d24387

Merge branch 'coupler_mp_notest' of github.com:rem1776/FMS into coupl…

f25e261

…er_mp_notest

many linter fixes :(

3813ce3

rem1776 added 4 commits August 25, 2023 17:10

Merge remote-tracking branch 'origin/mixedmode' into coupler_mp_notest

22a41dc

Add in class(*) for aof_set_coupler_flux optional reals

346c225

fix issues with checks for only one kind associated

039e34d

only check association if bc count is >0

c41238b

mlee03 reviewed Aug 28, 2023

View reviewed changes

rem1776 added 3 commits August 28, 2023 16:28

fix the checks again

d509e0d

cmake build

d6ae048

remove added prints

fea9409

mlee03 reviewed Aug 29, 2023

View reviewed changes

rem1776 added 2 commits August 29, 2023 12:20

add bc count > 0 to error checking

60d5bc5

Merge remote-tracking branch 'origin/mixedmode' into coupler_mp_notest

298f70b

rem1776 requested review from J-Lentz and mcallic2 August 29, 2023 18:44

mlee03 reviewed Aug 30, 2023

View reviewed changes

J-Lentz approved these changes Aug 30, 2023

View reviewed changes

Ryan Mulhall added 3 commits August 30, 2023 13:23

Merge remote-tracking branch 'upstream/mixedmode' into coupler_mp_notest

67ad65e

remove some other changes that slid in

47ae5f7

take out set check in spawn routines

27c6e87

mlee03 reviewed Aug 30, 2023

View reviewed changes

Ryan Mulhall and others added 5 commits August 30, 2023 14:29

build fixes

b6b1d02

fix build for real

41a5bd7

typo

c6fc1b4

another typo

0b501bd

one last thing hoepfully

6b91325

mlee03 approved these changes Aug 30, 2023

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coupler mixed precision #1353

coupler mixed precision #1353

rem1776 commented Aug 23, 2023

mlee03 Aug 25, 2023

rem1776 Aug 25, 2023

mlee03 Aug 25, 2023

rem1776 Aug 25, 2023

mlee03 Aug 25, 2023

rem1776 Aug 25, 2023

mlee03 Aug 25, 2023

rem1776 Aug 25, 2023

mlee03 Aug 25, 2023

rem1776 Aug 25, 2023

mlee03 Aug 25, 2023

rem1776 Aug 25, 2023

mlee03 Aug 28, 2023

rem1776 Aug 28, 2023 •

edited

Loading

rem1776 Aug 28, 2023 •

edited

Loading

mlee03 Aug 28, 2023

rem1776 Aug 29, 2023

mlee03 Aug 29, 2023

rem1776 Aug 30, 2023 •

edited

Loading

mlee03 commented Aug 29, 2023

mlee03 Aug 30, 2023

mlee03 Aug 30, 2023

rem1776 Aug 30, 2023 •

edited

Loading

J-Lentz Aug 30, 2023

rem1776 Aug 30, 2023

mlee03 Aug 30, 2023

mlee03 commented Aug 30, 2023

	if (present(mol_wt)) then
	call fm_util_set_value('mol_wt', mol_wt)
	else
	call fm_util_set_value('mol_wt', 0.0)
	endif

coupler mixed precision #1353

coupler mixed precision #1353

Conversation

rem1776 commented Aug 23, 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

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

rem1776 Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

rem1776 Aug 28, 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

rem1776 Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

mlee03 commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rem1776 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

mlee03 commented Aug 30, 2023

rem1776 Aug 28, 2023 •

edited

Loading

rem1776 Aug 28, 2023 •

edited

Loading

rem1776 Aug 30, 2023 •

edited

Loading

rem1776 Aug 30, 2023 •

edited

Loading