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/convert stat #344

Merged
merged 10 commits into from
Jun 12, 2020
Merged

Feature/convert stat #344

merged 10 commits into from
Jun 12, 2020

Conversation

clouden90
Copy link
Contributor

@clouden90 clouden90 commented May 5, 2020

Description

SOCA State Converter

What bug does it fix, or what feature does it add?
This application performs state conversion both horizontally and vertically. A few changes has been made based on the previous comments
.
The major change is now the intermediate grid step is removed. The mom_diag_remap module is used and the re-gridding is done in soca_gridgen step.

Now the application will do conversion based on the following steps:
(0) Pre-processing: run soca_gridgen to convert grid coordinate to z* grid for both source & target grid;
(1) Call soca_convertstate_setup to read pre-computed z* grid and original grid from soca_gridspec nc file for both source & target grid;
(2) Call soca_convertstate_change_resol to convert selected state variables (except target grid cell thickness, which has been defined in step (1)) by following steps:
a. Vertical remapping from source grid to source_z* grid for selected variable;
b. Horizontal remapping from source_z* grid to target_z* grid for selected variable;
c. Vertical remapping from target_z* grid to target grid for selected variable.

Several Issues:

  1. The horiz_interp_and_extrap_tracer() is the subroutine used for horizontal interpolation in MOM6 . The main problem we cannot directly use horiz_interp_and_extrap_tracer() is that it only supports tracer variables (e.g. T, S), but we need conversion of vector variables in our application. Also I found that fill_miss_2d may not work properly in the case that grid cell is surrounded by land mask.

  2. The example of how to use the state converter can be found in test case “test_soca_convertstate” and associated yaml file “convertstate.yml”. This case shows conversion from global domain (5deg) to coarser global grid (generated from test_soca_gridgen_small). I have done several tests to validate the accuracy/performance of the SOCA state converter and the results are well-documented in a separated report.

Issue(s) addressed

closes #309

Testing

How were these changes tested? Updated test_soca_convertstate (Global -> Coarse global grid)
What compilers / HPCs was it tested with? gcc 7.4, Intel 18, Intel 19
Are the changes covered by ctests? Yes

Dependencies

#341 , #339 and possibly #340, or #340 could be addressed here.
meshgrid and soca_fill_miss_2d will be removed after PR: Change meshgrid and fill_miss_2d to public #11
#348 (future work)

@aerorahul aerorahul requested a review from travissluka May 5, 2020 19:59
@clouden90 clouden90 removed the request for review from travissluka May 5, 2020 20:04
@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@17d30ce). Click here to learn what that means.
The diff coverage is 97.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #344   +/-   ##
==========================================
  Coverage           ?   95.99%           
==========================================
  Files              ?      102           
  Lines              ?     4119           
  Branches           ?        0           
==========================================
  Hits               ?     3954           
  Misses             ?      165           
  Partials           ?        0           
Impacted Files Coverage Δ
src/soca/Utils/soca_convert_stats_mod.F90 96.86% <96.86%> (ø)
src/soca/Geometry/soca_geom_mod.F90 100.00% <100.00%> (ø)
src/soca/State/soca_state.interface.F90 100.00% <100.00%> (ø)
src/soca/State/soca_state_mod.F90 98.96% <100.00%> (ø)
src/mains/GridGen.h 100.00% <0.00%> (ø)
test/executables/TestVariableChange.cc 100.00% <0.00%> (ø)
src/mains/3DVar.cc 100.00% <0.00%> (ø)
src/soca/GetValues/LinearGetValues.cc 92.68% <0.00%> (ø)
src/soca/Transforms/Balance/soca_kst_mod.F90 94.11% <0.00%> (ø)
src/soca/GeometryIterator/GeometryIterator.cc 72.41% <0.00%> (ø)
... and 96 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17d30ce...6372eb5. Read the comment docs.

@guillaumevernieres
Copy link
Contributor

guillaumevernieres commented May 5, 2020

@clouden90 , we need a few pr's to go in before we can think about merging that work. @travissluka and I will address #339 (#210 is in review) since this is going to break a few of our change of variables ... I think. Your work should be a little easier to fit in after #210 and #339 to then address and hopefully close #340 and #309, possibly enhancing whatever we'll come up with in #339.
Sounds like a plan?

@clouden90
Copy link
Contributor Author

@clouden90 , we need a few pr's to go in before we can think about merging that work. @travissluka and I will address #339 (#210 is in review) since this is going to break a few of our change of variables ... I think. Your work should be a little easier to fit in after #210 and #339 to then address and hopefully close #340 and #309, possibly enhancing whatever we'll come up with in #339.
Sounds like a plan?

@guillaumevernieres , sure, it sounds a perfect plan to me. I do see #210 before reopening this draft pull request. This draft PR is mainly for tracking purpose as well as exchanging ideas. I will also take a look #210 and other dependencies to see how I am going to fit in my work.

@clouden90 clouden90 requested a review from travissluka May 6, 2020 14:00
@clouden90
Copy link
Contributor Author

@travissluka , I accidentally removed you from the reviewer's list. Just added you back to keep you in the loop. No need actions now.

@clouden90 clouden90 force-pushed the feature/convert_stat branch 2 times, most recently from f64e03a to 7b34a13 Compare May 12, 2020 02:47
@clouden90 clouden90 self-assigned this May 12, 2020
@clouden90 clouden90 added the enhancement New feature or request label May 12, 2020
@clouden90 clouden90 force-pushed the feature/convert_stat branch from 7b34a13 to d4d7cea Compare May 13, 2020 01:59
@guillaumevernieres guillaumevernieres added this to the SOCA-May-2020 milestone May 13, 2020
@clouden90 clouden90 force-pushed the feature/convert_stat branch 2 times, most recently from da54b13 to 8884397 Compare May 19, 2020 00:04
@clouden90 clouden90 force-pushed the feature/convert_stat branch from 3cdabb4 to 4710d04 Compare May 20, 2020 22:29
! set hocn for src & des grid
self%hocn_src = hocn%val
hocn2%val = des%h
self%hocn_des = hocn2%val
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need hocn2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hocn2 is target grid cell thickness. We keep it here in case in the future if we have regional mom6 model setup and one would like to use the converted results to hotstart the model directly.

! set lon lat for src & des
if (field_src%name == "hocn" .and. field_des%name == "hocn" ) then
return
end if
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I lost the logic. Why do we return at that point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hocn has been set in soca_convertstate_setup so skip here.


end do

end subroutine soca_hinterp
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: Lost focus here, need to look in more detail at hinterp.

Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

@clouden90 , I went through most of the code and left a few comments. Please have a look.

Things that needs to be done before you spend more time on that work:

  1. Contact GFDL, or dig into MOM6, it seems to me that this feature already exists.
  2. We need two convert_state tests: fine -> coarse and coarse -> fine
  3. Since we have the exact solution of the 2 tests, we also need to compare the interpolated state to the truth.

That's a lot of work so thanks!

test/testref/convertstate.test Outdated Show resolved Hide resolved
@guillaumevernieres
Copy link
Contributor

@clouden90 , sorry for the travis-ci failures over the last few days. It should work now, please merge develop into your branch and push. If you are working in containers, update your image. It was updated a few days ago.

@clouden90 clouden90 marked this pull request as ready for review June 12, 2020 02:56
@guillaumevernieres guillaumevernieres added the waiting for another PR waiting on another pull request to be merged first label Jun 12, 2020
@@ -1,5 +1,5 @@
inputVariables:
variables: &soca_vars [tocn, socn, hocn, cicen, uocn, vocn]
variables: &soca_vars [ssh, tocn, socn, uocn, vocn, hocn, cicen]
inputresolution:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

All good besides minor comments. I revived issue #348 which I was suppose to address for your work and I didn't ... But we'll need to revisit in the future since only checking the size of the domain isn't good enough to figure out if 2 geometries are the same ... Subject of an other PR, "soon".

@guillaumevernieres guillaumevernieres merged commit 7d4359f into develop Jun 12, 2020
@guillaumevernieres guillaumevernieres deleted the feature/convert_stat branch June 12, 2020 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting for another PR waiting on another pull request to be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interpolate MOM6 restarts between domains
2 participants