The send_data math (field buffer update) functions. #1131

ngs333 · 2023-02-03T05:25:37Z

Description
This PR is largely for the field buffer updating math routines to be when selected in
a namelist option in the legacy diag manager. Key parts of the presented code is
intend to supports the modern diag manager out of the box.
Functionality that was originally in send_data_3d was refactored and moved
to their own functions. The added capability of for direct 3D and 4D fields (with
corresponding 4D and 5D) buffers used in the legacy and modern diag manager, respectively.
The functions support R4, R8, I4 and I8 fields and buffers (matching in type) ,
Integer versions of the functions are not allowed in the legacy diag manager and
are commented out for this review .

Most of the code refactored from send_data_3d is in these files:
1. fms_diag_fieldbuff_update.F90
2. include/fms_diag_fieldbuff_update.inc
3. include/fms_diag_fieldbuff_update.fh

To allow the math functions to work with both the legacy and the modern diag_manager,
some refactoring of bounds checking functions was also necessary. This was used also
as an opportunity to remove repetitive code, and some utilities related to array index bounding boxes were introduced. These functions are largely in diag_util.F90

Some conventions:
Some of the code has comments that begin with !TODO: .Those that on the same line also have
text (MDM) , I expect that they be resolved in the modern diag manager.

Some missed opportunities :

Given the existence of the R4 and R8 generated buffer update functions, it should be possible to
remove the copy of the field array used in send_data_3d that was introduced in the EMC mixed mode merge.
I was desired to set a pointer to the lowest level (elemental) buffer update math function.
There were issues implementing this (see this would have removed the innermost if/then/else construct in the buffer update loops. See fms_diag_outfield_type in file fms_daig_outfield.F90.

Fixes # (issue)

How Has This Been Tested?
All former unit tests pass. More unit tests were added. Regression tests are to be done.
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

also corrected new #define typo from last pull.

the lagacy diag_manager. Adds a refactored version of send_data_3d, modified the buffer bounds checking functions, and necessary changes in diag_data and diag_util.

…cs.F90). Fixed CMakeLists.txt; cleaned up fms_diag_outfield_mod.

meth functions.

…s_imp.

bointer remapping; plus moving back to original interface some diag_manager.F90 routines. Also removing send_dada_3d_refac() routine from diag_manager.F90.

…rue. Fixes bug of incorrect pointer bounds remapping (switching order of last two coordinates).

…ffer

sned_data_3d and missing some documentation.

conventions.

…into send_math_legacy_plus

unit test has the appropriate set access to private fields.

thomas-robinson

I still think you should remove modern from the names of the "modern" functions. If you want to use legacy for the old ones, I supposed that can be acceptable. There are some other comments/changes that need to be done still.

diag_manager/diag_data.F90

thomas-robinson · 2023-02-10T10:35:07Z

diag_manager/diag_manager.F90

+    TYPE(fmsDiagOutfield_type), ALLOCATABLE:: ofield_cfg !<Instance  used in calling math funcsions.
+    LOGICAL :: mf_result !<Logical result returned from some math (buffer udate) functions.
+    LOGICAL, DIMENSION(1,1,1), target :: mask_dummy
+    REAL :: rmask_threshold


what is this? Should it be a parameter?

The the current diag_manager send_data_3d, there are several uses of magic numbers 0.5_r4_kind and 0.5_r8_kind. The same value, whatever it is, needs to be passed to one of the math functions. I used READ rmask_threshold to hold and pass the value being used. I will update comments.

thomas-robinson · 2023-02-10T10:42:10Z

diag_manager/diag_util.F90

+    module procedure fms_update_bounds_legacy
+    module procedure fms_update_bounds_modern


Did we discuss this in an email? The modern and legacy still irk me.

thomas-robinson · 2023-02-10T10:45:35Z

diag_manager/diag_util.F90

+  !! of the "array" argument and store it the bounding box argument "bounds"
+  SUBROUTINE fms_bounds_from_array_4D(bounds, array)
+    REAL, INTENT( in), DIMENSION(:,:,:,:) :: array !< The 4D input array.
+    TYPE (fms_diag_ibounds_type), INTENT(inout) :: bounds !< The instance of the bounding box.


Why can't this be the constructor function? If you move this to diag_data, then the variables could be private.

thomas-robinson · 2023-02-10T10:46:49Z

diag_manager/diag_util.F90

+  !> @brief Update the the first three (normally  x, y, and z)  min and
+  !! max boundaries (array indices) of the input bounding box "bounds" with
+  !! the six specified bounds values.
+SUBROUTINE fms_update_bounds_modern(bounds, lower_i, upper_i, lower_j, upper_j, lower_k, upper_k)


I still don't like this name. We are going to have to go through and change this in the future. And by that time, everyone will forget.

thomas-robinson · 2023-02-10T13:49:48Z

diag_manager/include/fms_diag_fieldbuff_update.fh

+    FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:) :: field_ptr => null()!< Pointer to the field
+    FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:,:) :: ofb_ptr => null()!< Pointer to the outfield buffer.
+    FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:,:) :: ofc_ptr => null()!< Pointer to the outfield counter.
+    LOGICAL , pointer, DIMENSION(:,:,:,:) :: mask_ptr => null()!< Pointer to the mask.


We need to talk about these. It turns out there is an implied save here.

Please remove the =>null()

thomas-robinson · 2023-02-10T13:55:35Z

diag_manager/include/fms_diag_fieldbuff_update.fh

+        FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:) :: field_ptr => null() !< Pointer to the field
+        FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:,:) :: ofb_ptr => null() !< Pointer to the outfield buffer.
+        LOGICAL , pointer, DIMENSION(:,:,:,:) :: mask_ptr => null() !< !< Pointer to the mask.


We need to discuss these also...

thomas-robinson · 2023-02-10T13:56:33Z

diag_manager/include/fms_diag_fieldbuff_update.fh

+!> @brief This code will be used by the preprecessor to generate an implementation
+!! to the module procudure for the fieldbuff_copy_fieldvals interface.
+!! The function may set or add to the output field buffer (argument ofb) with the input
+!! field data array  (argument field)


Yes but will that show up as the result when the doxygen documentation is created?

test_fms/diag_manager/test_diag_update_buffer.F90

diag_manager/diag_util.F90

ganganoaa · 2023-02-10T15:43:34Z

diag_manager/diag_util.F90

+      bounds%kmax = UBOUND(array,3)
+  END SUBROUTINE  fms_bounds_from_array_4D
+
+ !> @brief Determine the bounds of the first three dimentions


typo in dimentions

diag_manager/diag_util.F90

ganganoaa · 2023-02-10T16:16:40Z

diag_manager/diag_util.F90


    CHARACTER(len=128)  :: error_string1, error_string2
+    LOGICAL :: lims_not_exact = .true.


Does it imply save on lims_not_exact ?

Yes, this should be:

LOGICAL :: lims_not_exact lims_not_exact = .true.

OR you don't need it at all because 2 lines down, you are setting lims_not_exact anyway. Are you trying to save this variable for next time?

diag_manager/fms_diag_outfield.F90

diag_manager/fms_diag_time_reduction.F90

fms_diag_fieldbuff_update.fh; and added TODO: with explanation. Added to diag manager namelist. Fixed some comments as per PR review.

project standard.

thomas-robinson

If you are going to insist on leaving the variables in fmsDiagIbounds_type public, then it needs to be documented in the code why they are public.

(fms_diag_fieldbuff_update.fh, diang_manager.F90)Please remove => null() on any new pointers that are initialized when they are declared:

type(something), pointer :: var => null()

Instead do something like this:

type(something), pointer :: var
var => null()

This is a question about the variable lims_not_exact that should be addressed

Please add a TODO for the integer pow_value so we can discuss it with the rewrite

There still some doxygen that needs to be addressed in fms_diag_fieldbuff_update.fh

thomas-robinson · 2023-02-21T10:46:28Z

diag_manager/diag_data.F90

+  INTEGER :: imin !< Lower i bound.
+  INTEGER :: imax !< Upper i bound.
+  INTEGER :: jmin !< Lower j bound.
+  INTEGER :: jmax !< Upper j bound.
+  INTEGER :: kmin !< Lower k bound.
+  INTEGER :: kmax !< Upper k bound.


You need to add documentation about why these variables are public because it goes against the FMS coding standards.

thomas-robinson · 2023-02-21T10:50:12Z

diag_manager/diag_manager.F90

-    LOGICAL, DIMENSION(:,:,:), INTENT(in), OPTIONAL :: mask
-    CLASS(*), DIMENSION(:,:,:), INTENT(in), OPTIONAL :: rmask
+    LOGICAL, DIMENSION(:,:,:), INTENT(in), OPTIONAL, contiguous, target :: mask
+    CLASS(*), DIMENSION(:,:,:), INTENT(in), OPTIONAL, target :: rmask
    CHARACTER(len=*), INTENT(out), OPTIONAL :: err_msg



@ganganoaa We don't require updating old code/variables. It's too onerous on developers because, like you said, it's not obvious what some of the variables are.

thomas-robinson · 2023-02-21T10:54:04Z

diag_manager/diag_manager.F90

+    REAL(kind=r4_kind), POINTER, DIMENSION(:,:,:) :: rmask_ptr_r4 => null() !< A pointer to r4 type of rmask
+    REAL(kind=r8_kind), POINTER, DIMENSION(:,:,:) :: rmask_ptr_r8 => null() !<A pointer to r8 type of rmask


Remove the => null()

This still needs to be fixed

thomas-robinson · 2023-02-21T10:58:55Z

diag_manager/diag_util.F90


    CHARACTER(len=128)  :: error_string1, error_string2
+    LOGICAL :: lims_not_exact = .true.


Yes, this should be:

LOGICAL :: lims_not_exact lims_not_exact = .true.

OR you don't need it at all because 2 lines down, you are setting lims_not_exact anyway. Are you trying to save this variable for next time?

thomas-robinson · 2023-02-21T11:03:15Z

diag_manager/include/fms_diag_fieldbuff_update.fh

+    FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:) :: field_ptr => null()!< Pointer to the field
+    FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:,:) :: ofb_ptr => null()!< Pointer to the outfield buffer.
+    FMS_DIAG_FBU_DATA_TYPE_ , pointer, DIMENSION(:,:,:,:,:) :: ofc_ptr => null()!< Pointer to the outfield counter.
+    LOGICAL , pointer, DIMENSION(:,:,:,:) :: mask_ptr => null()!< Pointer to the mask.


Please remove the =>null()

thomas-robinson · 2023-02-21T11:07:23Z

diag_manager/include/fms_diag_fieldbuff_update.inc

@@ -0,0 +1,31 @@
+#undef FMS_DIAG_FBU_DATA_TYPE_


@mlee03 can you look at this and make sure it's following your standards.

We've been using "kind" macros instead of "type", e.g, FMS_DIAG_FBU_DATA_KIND_ r4_kind. Also we put the macro definitions in the _r4.fh and _r8.fh files (separate files for r4 and r8_kind); the code went into the *.inc files.

mlee03 · 2023-02-22T13:07:16Z

diag_manager/include/fms_diag_fieldbuff_update.inc

@@ -0,0 +1,31 @@
+#undef FMS_DIAG_FBU_DATA_TYPE_


We've been using "kind" macros instead of "type", e.g, FMS_DIAG_FBU_DATA_KIND_ r4_kind. Also we put the macro definitions in the _r4.fh and _r8.fh files (separate files for r4 and r8_kind); the code went into the *.inc files.

diag_manager/include/fms_diag_fieldbuff_update.fh

mlee03 · 2023-02-22T13:26:30Z

diag_manager/include/fms_diag_fieldbuff_update.inc

@@ -0,0 +1,31 @@
+#undef FMS_DIAG_FBU_DATA_TYPE_


We also put in the GNU Lesser General Public License stuff in the *inc and *fh files

data members of fmsDiagIbounds_type.

…egacy_plus

…h_legacy_plus

ngs333 · 2023-02-22T18:12:29Z

If you are going to insist on leaving the variables in fmsDiagIbounds_type public, then it needs to be documented in the code why they are public.

(fms_diag_fieldbuff_update.fh, diang_manager.F90)Please remove => null() on any new pointers that are initialized when they are declared:
type(something), pointer :: var => null()
Instead do something like this:
type(something), pointer :: var
var => null()
This is a question about the variable lims_not_exact that should be addressed

Please add a TODO for the integer pow_value so we can discuss it with the rewrite

There still some doxygen that needs to be addressed in fms_diag_fieldbuff_update.fh

These items from @thomas-robinson were addressed in the push of 1PM today; but for exception of the doxygen that needs to be addressed in fms_diag_fieldbuff_update.fh, which is coming shortly.

fms_diag_fieldbuff_update.fh

thomas-robinson

These are the specific places that needs to be addressed. There may be more => null() too.

diag_manager.F90 L1494 1495 need to remove the => null()

diag_util.F90 L1048 the lims_not_exact variable should not be initialized. See the comment in the review.

fms_diag_fieldbuff_update.fh L882 Needs a doxygen documentation

thomas-robinson · 2023-02-23T11:55:35Z

diag_manager/diag_manager.F90

+    REAL(kind=r4_kind), POINTER, DIMENSION(:,:,:) :: rmask_ptr_r4 => null() !< A pointer to r4 type of rmask
+    REAL(kind=r8_kind), POINTER, DIMENSION(:,:,:) :: rmask_ptr_r8 => null() !<A pointer to r8 type of rmask


This still needs to be fixed

declaration line, and some missing comments.

Miguel R Zuniga and others added 30 commits July 25, 2022 14:44

Added initialization of certain structures to test_output_yaml.F90.

6fa06b7

Added initialization of certain structures to test_output_yaml.F90;

c7b4687

also corrected new #define typo from last pull.

Merge branch 'NOAA-GFDL:main' into main

03b3288

Merge branch 'NOAA-GFDL:main' into main

cca95af

Merge branch 'NOAA-GFDL:main' into main

ab326c5

Merge branch 'NOAA-GFDL:main' into main

92f5067

Merge branch 'NOAA-GFDL:main' into main

bc688a5

Merge branch 'NOAA-GFDL:main' into main

47fb5a4

Merge branch 'NOAA-GFDL:main' into main

46a4b4b

Adding code from previous work.

f120ca2

Added math (field buffer update) functions that use 4D fields to

f3ddae9

the lagacy diag_manager. Adds a refactored version of send_data_3d, modified the buffer bounds checking functions, and necessary changes in diag_data and diag_util.

First version that compiles (with gcc).

a7bc290

Moved elemental math function to own module (fms_diag_elem_weight_pro…

46c909c

…cs.F90). Fixed CMakeLists.txt; cleaned up fms_diag_outfield_mod.

Shortening source code line length below 120 lines.

cd133a3

First compiling version with refactored_send option and refactored

e860e3d

meth functions.

Fixing code line lenght > 120 chars; Fixing reference to update_bound…

08cacb8

…s_imp.

Changes to replace ALLOCATABLE in certain lower-level functions doing

8f819cb

bointer remapping; plus moving back to original interface some diag_manager.F90 routines. Also removing send_dada_3d_refac() routine from diag_manager.F90.

First version passing all unit tests but without USE_REFACTORED_SEND.

4324b3e

First version with all unit tests passing with use_refactored_send==t…

79cd026

…rue. Fixes bug of incorrect pointer bounds remapping (switching order of last two coordinates).

Removing reference to unavailable module from test_disg_update_buffer.

4430ab2

Removing reference to another unavail module in test_diag_update_buffer.

91049f5

Removed references to modern diag structures from test_diag_update_bu…

f8ada45

…ffer

Removing repeating warning mesage.

5dbee60

Removing repeating output temporarily used in debugging.

71bbd48

Removing note at end of test (unexplicably) causing unit test failure.

8eed227

Fixing test_diag_update_buffer : setting unitialized vars.

7e51b8f

Completed working version, but missing removal of copy of field_out in

c9c38d5

sned_data_3d and missing some documentation.

Improved documentation in various places. Improved following of naming

8190672

conventions.

Fixing line longer than 120 chars. Improved some comments.

2b45ca6

Merge branch 'NOAA-GFDL:main' into main

cfaccac

ngs333 added 4 commits February 8, 2023 18:57

Numerous changes based oncode review.

87adf19

Merge branch 'send_math_legacy_plus' of https://github.com/ngs333/FMS …

ca10d14

…into send_math_legacy_plus

Addresses more items from the review.

beebd46

Adding function initialize_for_ut to fmsDiagOutfield_type so

7aba81b

unit test has the appropriate set access to private fields.

thomas-robinson requested changes Feb 10, 2023

View reviewed changes

ganganoaa reviewed Feb 10, 2023

View reviewed changes

Changes originating from PR review comments.

2b688dc

ganganoaa previously approved these changes Feb 10, 2023

View reviewed changes

Several modificationsand comment updates based on review.

c2615ba

ngs333 dismissed ganganoaa’s stale review via c2615ba February 12, 2023 04:57

ngs333 added 5 commits February 12, 2023 00:05

Fixing a line length > 120 columns.

39ffc6e

Fixing typo of remaning rmaks to mask in last lint fix push.

004ad06

Fixing more items from PR review.

9ff1c78

Localized used of dummy arrays for pointer_bounds_remapping issue into

95e5e2c

fms_diag_fieldbuff_update.fh; and added TODO: with explanation. Added to diag manager namelist. Fixed some comments as per PR review.

Fixing whitespace at end of file lint error; reformatting two files to

3a0a674

project standard.

thomas-robinson requested changes Feb 21, 2023

View reviewed changes

mlee03 reviewed Feb 22, 2023

View reviewed changes

ngs333 added 3 commits February 22, 2023 12:37

Updates based on latest review comments, mostly making private the

f2b5b6d

data members of fmsDiagIbounds_type.

Merge branch 'main' of https://github.com/ngs333/FMS into send_math_l…

8047cea

…egacy_plus

Merge branch 'main' of https://github.com/NOAA-GFDL/FMS into send_mat…

2653656

…h_legacy_plus

ngs333 added 2 commits February 22, 2023 16:25

Added module fms_diag_bbox_mod; improved comments in

f31d685

fms_diag_fieldbuff_update.fh

Fixing type in CMakeLists.txt

76a2241

thomas-robinson requested changes Feb 23, 2023

View reviewed changes

ngs333 added 2 commits February 23, 2023 10:41

Modifications from PR review; mostly not setting pointers to null at

1583e90

declaration line, and some missing comments.

Fixing spelling in two comment lines.

8faab40

thomas-robinson approved these changes Feb 23, 2023

View reviewed changes

rem1776 approved these changes Feb 23, 2023

View reviewed changes

rem1776 merged commit 6255971 into NOAA-GFDL:main Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The send_data math (field buffer update) functions. #1131

The send_data math (field buffer update) functions. #1131

ngs333 commented Feb 3, 2023 •

edited by thomas-robinson

Loading

thomas-robinson left a comment

thomas-robinson Feb 10, 2023

ngs333 Feb 10, 2023 •

edited

Loading

thomas-robinson Feb 10, 2023

thomas-robinson Feb 10, 2023

thomas-robinson Feb 10, 2023

thomas-robinson Feb 10, 2023

thomas-robinson Feb 21, 2023

thomas-robinson Feb 10, 2023

thomas-robinson Feb 10, 2023

ganganoaa Feb 10, 2023

ganganoaa Feb 10, 2023

thomas-robinson Feb 21, 2023

thomas-robinson left a comment

thomas-robinson Feb 21, 2023

thomas-robinson Feb 21, 2023

thomas-robinson Feb 21, 2023

thomas-robinson Feb 23, 2023

thomas-robinson Feb 21, 2023

thomas-robinson Feb 21, 2023

thomas-robinson Feb 21, 2023

mlee03 Feb 22, 2023

mlee03 Feb 22, 2023

mlee03 Feb 22, 2023

ngs333 commented Feb 22, 2023

thomas-robinson left a comment

thomas-robinson Feb 23, 2023

		module procedure fms_update_bounds_legacy
		module procedure fms_update_bounds_modern


		CHARACTER(len=128) :: error_string1, error_string2
		LOGICAL :: lims_not_exact = .true.

		REAL(kind=r4_kind), POINTER, DIMENSION(:,:,:) :: rmask_ptr_r4 => null() !< A pointer to r4 type of rmask
		REAL(kind=r8_kind), POINTER, DIMENSION(:,:,:) :: rmask_ptr_r8 => null() !<A pointer to r8 type of rmask

The send_data math (field buffer update) functions. #1131

The send_data math (field buffer update) functions. #1131

Conversation

ngs333 commented Feb 3, 2023 • edited by thomas-robinson Loading

thomas-robinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngs333 Feb 10, 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

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

ngs333 commented Feb 22, 2023

thomas-robinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngs333 commented Feb 3, 2023 •

edited by thomas-robinson

Loading

ngs333 Feb 10, 2023 •

edited

Loading