Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce memory usage during GWDO stats estimation #1235

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

abishekg7
Copy link
Collaborator

A copy of #1228 with branch renamed. This PR attempts to improve the memory footprint during the computation of Gravity Wave Drag Orography statistics in the pre-processing step.

The previous approach relied on each MPI rank reading all of the topography and land use tiles, and hence would run out of memory before we could fully subscribe to all cores in a node. In the new approach, we only read in one tile at a time and when we encounter a pixel whose data is not already available in a linked list. The get_box subroutine is modified to call get_tile_from_box_point to check if the current pixel in the box is present in the linked list, and if not, it appends this tile (after reading both topo and land use data) to the head of the list.

This PR also changes box, box_landuse, dxm, nx and ny to be local variables, instead of module variables. This provides a little better readability, along with advantages of thread safety, etc.

@abishekg7
Copy link
Collaborator Author

Tested on Eris (GNU) with 24km grid, and Derecho (GNU) with 15km and 24km. Derecho NVHPC build and run is successful for 15km grid.

@abishekg7
Copy link
Collaborator Author

Tested on 5km grid as well.

Copy link
Contributor

@mgduda mgduda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done some testing with a 120-km global mesh and all seems good. I'll do further testing with a variable-resolution regional mesh. Meanwhile, I've left a few comments that mostly concern style.

src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
src/core_init_atmosphere/mpas_init_atm_gwd.F Outdated Show resolved Hide resolved
! computes the mean elevation in the array and stores that value in
! the module variable 'box_mean'.
! the module variable 'box_mean'. 'tilesHead' points to the head of the linked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

box_mean (and also box, above) are no longer module variables, so it would be good to reflect this in the comments.

!
call get_box(latCell(iCell)*rad2deg, lonCell(iCell)*rad2deg, dc)
call get_box_size_from_lat_lon(latCell(iCell)*rad2deg, lonCell(iCell)*rad2deg, dc, nx, ny)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the cell coordinates are changed from degrees to radians inside the get_box_size_from_lat_lon routine. Would it be simpler to not convert from radians to degrees here and to also eliminate the reverse conversions inside the routine?

! and the latitude and longitude coordinates (radians)
!
!-----------------------------------------------------------------------
subroutine get_box_size_from_lat_lon(lat, lon, dx, nx, ny)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like lon is actually needed by this routine, which makes sense: the box size should be independent of longitude and should only depend on latitude.

real (kind=RKIND), intent(inout) :: box_mean ! Mean value of topography in box

type(mpas_gwd_tile_type), pointer :: thisTile
integer :: i, j, ii, jj, ic, jc, ix, jx, tile_offset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like tile_offset is ever used.


integer, parameter :: tile_x = 1200 ! x-dimension of each tile of global 30-arc-second landuse
integer, parameter :: tile_y = 1200 ! y-dimension of each tile of global 30-arc-second landuse

integer (c_int) :: istatus
integer :: ix, iy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the changes in this PR, ix and iy are no longer used in this function.


implicit none

character(len=*), intent(in) :: path

character(len=*), intent(in) :: sub_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like sub_path is used.

end function get_tile_from_box_point



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest only two blank lines here for consistency.

end subroutine get_box_size_from_lat_lon



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest two blank lines.

iErr = 0

end function free_tile_list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, let's use two blank lines.


deallocate(tile)

iErr = 0

end function read_global_30s_topo

end function read_30s_topo_tile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a second blank line after this function for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants