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

Feature/use_hash_indg #295

Merged
merged 14 commits into from
Aug 26, 2020
Merged

Feature/use_hash_indg #295

merged 14 commits into from
Aug 26, 2020

Conversation

weiyuan-jiang
Copy link
Contributor

Keep original i_indg and j_indg and use has_indg and hash_j_indg for the computation

@gmao-rreichle
Copy link
Contributor

replaces pull request #270

@@ -2738,10 +2738,10 @@ subroutine write_smapL4SMaup( option, date_time, work_path, exp_id, N_ens, &
data_h_9km_tile_8 = real(nodata_generic,kind(0.0D0)) ! init (not in grid2tile!)
data_v_9km_tile_8 = real(nodata_generic,kind(0.0D0)) ! init (not in grid2tile!)

call grid2tile( tile_grid_g, N_catf, tile_coord_f%i_indg, tile_coord_f%j_indg, data_h_9km_grid_8, &
call grid2tile( tile_grid_g, N_catf, tile_coord_f%hash_i_indg, tile_coord_f%hash_j_indg, data_h_9km_grid_8, &
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this should be %i_indg, not %hash_i_indg. Here, we are simply going back and forth between the tile space and the grid that is associated with this tile space. Because this subroutine only works for the EASE grid, and because %i_indg and %hash_i_indg are identical in this case, the results are going to be the same. But we still need to use the appropriate field of the tile_coord_f structure.

data_h_9km_tile_8 )

call grid2tile( tile_grid_g, N_catf, tile_coord_f%i_indg, tile_coord_f%j_indg, data_v_9km_grid_8, &
call grid2tile( tile_grid_g, N_catf, tile_coord_f%hash_i_indg, tile_coord_f%hash_j_indg, data_v_9km_grid_8, &
Copy link
Contributor

@gmao-rreichle gmao-rreichle Aug 18, 2020

Choose a reason for hiding this comment

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

Same comment as above. Two more such instances follow below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel we should not cheery-picking but be consistent through out the codes: hash_x_indg is used for computation and x_indg is used to keep the original record whenever we can. The only place we should use X_indg probably is here

if( isCubed ) then ! cs grid
! i_indg and j_indg are changed to LatLon grid
do k=1,N_cat
i1(k) = tile_coord(k)%i_indg
j1(k) = tile_coord(k)%j_indg
enddo

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't cherry-picking. For a grid2tile operation, we need to use the i,j indices that are associated with grid that is involved in the operation. Here, the grid is the original grid associated with the tile space, not the "hash" grid used in the perturbations and to support the EnKF analysis. I know this is a bit subtle because it looks like it's part of the EnKF analysis, but it's really just a complicated special case to output (1d array) tile-space fields in gridded (2d array) format (taking advantage of the fact that by construction the EASE-based tile space has at most one tile in each grid cell.)
Again, the grid involved here is NOT the hash grid that is used for perturbations or to support the EnKF analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems it is just part of SMAP writing. Changed

Copy link
Contributor Author

@weiyuan-jiang weiyuan-jiang left a comment

Choose a reason for hiding this comment

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

It needs to assign hash_x_indg here. Later on, it calls get_tile_grid which needs hash_x_indg

@GEOS-ESM GEOS-ESM deleted a comment from weiyuan-jiang Aug 18, 2020
@gmao-rreichle
Copy link
Contributor

The previous comment (by @weiyuan-jiang ) applies to pre-processing.
After discussion, it was agreed that the "%hash_[x]indg" fields should not be needed in pre-processing and should be initialized to no-data.
To resolve this, the interface to "get_tile_grid()" needs to be changed to accommodate using the subroutine in two different ways. One possibility is to add an optional argument that tells the subroutine whether to use %hash
[x]_indg or %[x]_indg.
Another possibility is to pass in the required integer and real arrays instead of the tile_coord structure.
More elegant options may exist.

weiyuan-jiang and others added 4 commits August 18, 2020 14:29
- fix of earlier change: tile2grid_full() should use %[x]_indg, not %hash_[x]_indg
- changed interface of several helper subroutines for mapping in TileCoordRoutines.F90:
   use generic [indg] vectors so that subroutines can be called with %[x]_indg or %hash_[x]_indg
@weiyuan-jiang
Copy link
Contributor Author

The program crashes. Need to figure out why

@gmao-rreichle gmao-rreichle merged commit 82a0650 into develop Aug 26, 2020
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.

convert %cs_i_indg and %cs_j_indg to simple arrays in Assim GridComp
2 participants