Add diag_manager testing for openmp #1281

uramirez8707 · 2023-07-11T21:33:43Z

Description

Adds testing for openmp
Updates code to prevent a race condition when allocating the data buffer and setting the flags

Fixes # (issue)

How Has This Been Tested?
CI

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

* fix - gnu and pgi issue with class(*) in send_data3d (NOAA-GFDL#1149) * feat: extend `string` interface in `fms_string_utils_mod` (NOAA-GFDL#1142) * fix: add omp directives for race condition in tridiagonal (NOAA-GFDL#1109) * fix: missing if statement in PR NOAA-GFDL#1149 (NOAA-GFDL#1155) * fix: time_manager missing changes from year to yr, month to mo, day to dy (NOAA-GFDL#1169) * chore: update changelog and version numbers for release (NOAA-GFDL#1167) * chore: append dev to version number post-release (NOAA-GFDL#1179) --------- Co-authored-by: Niki Zadeh <[email protected]> Co-authored-by: Jesse Lentz <[email protected]> Co-authored-by: ganganoaa <[email protected]> Co-authored-by: MiKyung Lee <[email protected]> Co-authored-by: Ryan Mulhall <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

test_fms/diag_manager/test_diag_manager2.sh

… old diag manager, fix possible race condition

ganganoaa

Some changes may be required but not critical to code function.

ganganoaa · 2023-07-21T13:04:20Z

diag_manager/fms_diag_field_object.F90

@@ -484,7 +477,6 @@ logical function allocate_data_buffer(this, input_data, diag_axis)
      call mpp_error ("allocate_data_buffer","The data input to set_data_buffer for "//&
        trim(this%varname)//" is not a supported type",  FATAL)
  end select
-!$omp end single
  allocate_data_buffer = allocated(this%data_buffer)


Is it good to add the following statement here?
this%set_data_buffer_is_allocated(allocated(this%data_buffer))

ganganoaa · 2023-07-21T13:07:31Z

diag_manager/fms_diag_field_object.F90

+  class (fmsDiagField_type) , intent(inout) :: this                     !< The field object
+  logical,                    intent (in)   :: data_buffer_is_allocated !< Flag saying that the math
+                                                                        !! functions need to be done
+


Comment for data_buffer_is_allocated is misleading.

@uramirez8707 you can tell us the .true. means the data buffer is allocated. it seems obvious but not all of the variables are always obvious

ganganoaa · 2023-07-21T13:41:45Z

diag_manager/fms_diag_object.F90

-    call this%FMS_diag_fields(diag_field_id)%set_data_buffer(field_data, FMS_diag_object%diag_axis,&
-                                                             is, js, ks, ie, je, ke)
+
+!> Only 1 thread allocates the output buffer amd sets set_math_needs_to_be_done


ganganoaa · 2023-07-21T14:16:07Z

diag_manager/fms_diag_object.F90

+!> Only 1 thread allocates the output buffer amd sets set_math_needs_to_be_done
+!$omp single
+    data_buffer_is_allocated = &
+      this%FMS_diag_fields(diag_field_id)%allocate_data_buffer(field_data, FMS_diag_object%diag_axis)


We need to avoid use of global variables/objects as much as we can. There could be multiple global instances of fmsDiagObject_type. Here, diag_axis is associated to an instance this but not to any other instance. Every time we change the global instance of the fmsDiagObject_type we have to come back here and change accordingly which is not good. So, FMS_diag_object%diag_axis should be replaced by this%diag_axis.

yes! Good catch! Thanks.

ganganoaa · 2023-07-21T14:32:55Z

diag_manager/fms_diag_field_object.F90

-    this%data_buffer_allocated =  allocate_data_buffer(this, input_data, diag_axis)
-  if (.not.this%data_buffer_allocated) &
+  integer :: is, js, ks !< Starting indicies of the field_data relative to the global domain
+  integer :: ie, je, ke !< Ending indicied of the field_data relative to the global domain


typo indicied

ganganoaa · 2023-07-21T14:33:12Z

diag_manager/fms_diag_field_object.F90

  class (fmsDiagField_type) , intent(inout):: this !< The field object
  class(*), dimension(:,:,:,:), intent(in) :: input_data !< The input array
-  class(fmsDiagAxisContainer_type),intent(in)   :: diag_axis(:)          !< Array of diag_axis
-  integer :: is, js, ks !< Starting indicies of the field_data
-  integer :: ie, je, ke !< Ending indicied of the field_data


typo indicied

…th land openmp code

thomas-robinson · 2023-07-24T18:27:23Z

diag_manager/fms_diag_field_object.F90

+  class (fmsDiagField_type) , intent(inout) :: this                     !< The field object
+  logical,                    intent (in)   :: data_buffer_is_allocated !< Flag saying that the math
+                                                                        !! functions need to be done
+


@uramirez8707 you can tell us the .true. means the data buffer is allocated. it seems obvious but not all of the variables are always obvious

thomas-robinson · 2023-07-24T18:28:10Z

diag_manager/fms_diag_object.F90

-                                                             is, js, ks, ie, je, ke)
+
+!> Only 1 thread allocates the output buffer and sets set_math_needs_to_be_done
+!$omp critical


urg but agreed

thomas-robinson · 2023-07-24T18:29:58Z

diag_manager/fms_diag_field_object.F90

  axis_loop: do a = 1,naxes
    axis_id => this%axis_ids(a)
    select type (axis => diag_axis(axis_id)%axis)
      type is (fmsDiagFullAxis_type)
        length(a) = axis%axis_length()
    end select
  enddo axis_loop
-!> On a single thread, allocate the data buffer to the correct kind and size
-!$omp single
+


This doesnt need to be in a single/critical section?

This function is called in a critical section, so there is no need for it here.

uramirez8707 and others added 12 commits April 6, 2023 14:23

add openmp test

979ee90

fix to run openmp tests?

1644764

Fix so that the openmp test works in the CI

3761ff4

fix?

af58aa3

wut

dc8a8a1

ok ok

748dfe8

clean up

d3acfae

extend to 1d/2d/3d

f250609

revert previous change

21c77b5

Fix lint

84a982c

fix for the 3d case

fa55ccc

uramirez8707 requested a review from thomas-robinson as a code owner July 11, 2023 21:33

uramirez8707 added 2 commits July 12, 2023 10:23

minor correction to the indices

216a4fe

Merge branch 'dmUpdate' of github.com:NOAA-GFDL/FMS into dm_openmp

04103f8

thomas-robinson requested changes Jul 13, 2023

View reviewed changes

test_fms/diag_manager/test_diag_manager2.sh Show resolved Hide resolved

uramirez8707 added 5 commits July 13, 2023 08:17

set OMP_NUM_THREADS back to 1 after running tests

20227ba

Merge branch 'dmUpdate' of github.com:NOAA-GFDL/FMS into dm_openmp

8292442

fix the diag_table.yaml for the openmp test after the yaml update

3948497

Merge branch 'dmUpdate' of github.com:NOAA-GFDL/FMS into dm_openmp

ac2b183

Merge branch 'dmUpdate' of github.com:uramirez8707/FMS into dm_openmp

763141d

uramirez8707 marked this pull request as draft July 20, 2023 15:40

Fix the indices in the test + code to match what was happening in the…

d6f81cb

… old diag manager, fix possible race condition

uramirez8707 requested a review from ganganoaa July 20, 2023 19:03

uramirez8707 marked this pull request as ready for review July 20, 2023 19:03

ganganoaa reviewed Jul 21, 2023

View reviewed changes

uramirez8707 added 4 commits July 21, 2023 12:23

small refactor + fix typos

bfb4ab5

more refactoring

f9b35be

Refactor the openmp section, with omp critical so that it can work wi…

6ff7e97

…th land openmp code

Merge branch 'dmUpdate' of github.com:NOAA-GFDL/FMS into dm_openmp

45b7d0d

minor fix

069b360

thomas-robinson requested changes Jul 24, 2023

View reviewed changes

uramirez8707 and others added 2 commits July 31, 2023 11:52

documentation update

ffd4d6d

documentation update

b4e4d9b

thomas-robinson approved these changes Jul 31, 2023

View reviewed changes

thomas-robinson merged commit 7c763af into NOAA-GFDL:dmUpdate Jul 31, 2023
27 checks passed

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

test: Add diag_manager testing for openmp (NOAA-GFDL#1281)

bd853c2

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

test: Add diag_manager testing for openmp (NOAA-GFDL#1281)

8d74e47

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

test: Add diag_manager testing for openmp (NOAA-GFDL#1281)

3dd1921

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

test: Add diag_manager testing for openmp (NOAA-GFDL#1281)

c763503

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024

test: Add diag_manager testing for openmp (NOAA-GFDL#1281)

2429914

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add diag_manager testing for openmp #1281

Add diag_manager testing for openmp #1281

uramirez8707 commented Jul 11, 2023 •

edited

Loading

ganganoaa left a comment

ganganoaa Jul 21, 2023

ganganoaa Jul 21, 2023

thomas-robinson Jul 24, 2023

ganganoaa Jul 21, 2023

ganganoaa Jul 21, 2023

uramirez8707 Jul 21, 2023

ganganoaa Jul 21, 2023

ganganoaa Jul 21, 2023

thomas-robinson Jul 24, 2023

thomas-robinson Jul 24, 2023

thomas-robinson Jul 24, 2023

uramirez8707 Jul 25, 2023

Add diag_manager testing for openmp #1281

Add diag_manager testing for openmp #1281

Conversation

uramirez8707 commented Jul 11, 2023 • edited Loading

ganganoaa 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

uramirez8707 commented Jul 11, 2023 •

edited

Loading