mpas rttov #95

hkershaw-brown · 2020-12-11T18:13:30Z

Aim of this pull request is to commit the work Nancy and Soyoung did on MPAS and RTTOV.

Fixes https://github.com/NCAR/DART_development/issues/119

The rttov obs_def changes:

E_ALLMSG used instead of E_MSG
loc_sea variable used to create sfc, 2m, 10m locations relative to model surface elevation
monotonically decreasing pressure from TOA check now has greater than or equal to ( this is the bug fix)

The mpas_atm model_mod changes:

Debugging info in the comment at the start of the module.
New error code for pressure not monotonically decreasing with level
QTY_CLOUD_FRACTION added. Force non-negative hydrometeors and for QTY_CLOUD_FRACTION <1
compute_scalar_with_barycentric passing tavs(1), values(1,:). when n=1. There is some discussion of this in https://github.com/NCAR/DART_development/issues/119. It is a bit of a gotcha, so is on the list of things to tidy up in the mpas_atm module.
theta_to_tk has debugging info for non-zero istatus going in

There is some discussion on VERTISUNDEFINED in the comments (whether there should be the error code 82).

Tested these changes on Cheyenne with a 10 member version on Soyoung's mpas case. Note it was a 10 member test since each restart is 3.7GB. /glade/p/cisl/dares/hkershaw/mpas_rttov_testing

Check "pressure is monotonically increasing from TOA down" requires => rather than just >.

Using the model QTY_SURFACE_ELEVATION

The mpas-rttov debugging is descriped in this issue: NCAR/DART_development#119

correct loop naming for members loop

Discussion here: NCAR/DART_development#119

the comment on debugging info

timhoar

Could delete some commented-out lines.
I'm not a fan of the "Ha: " ... the comments are usually not informative, and git blame shows which lines were modified, when, and with the commit message.

models/mpas_atm/model_mod.f90

VERTISUNDEF locations often come in with missing_r8 as the vertical Note there is an early return in vert_convert_distrib_state when VERTISUNDEF so I am not sure which errors the istatus 82 was catching

timhoar · 2020-12-14T16:05:40Z

I was puzzling over the skintemp interpolation

       call compute_surface_data_with_barycentric(skintemp(:), location, expected_obs(1), istatus(1))

and am concerned that the skintemp is metadata common to all ensemble members. Ditto for seaice.
That doesn't seem right to me. Has anyone talked to Soyoung about this?

nancycollins · 2020-12-11T20:49:26Z

models/mpas_atm/model_mod.f90

      else
         write(string1,*) 'interp routine returned a good status but set value to MISSING_R8'
      endif

-      call error_handler(E_ERR,'model_interpolate', string1, source,revision,revdate, &
+      !call error_handler(E_ERR,'model_interpolate', string1, source,revision,revdate, &


edit: sorry! i just reread what section this is in. if the code is returning a good status but missing_r8, it should halt here because that's a bug. i'd remove the E_ALLMSG line and return it to using E_ERR in this case.

nancycollins · 2020-12-11T20:54:38Z

models/mpas_atm/model_mod.f90

@@ -4533,7 +4595,7 @@ subroutine convert_vert_distrib(state_handle, ens_size, location, obs_kind, ztyp
 ! if the entire ensemble has missing vertical values we can return now.
 ! otherwise we need to continue to convert the members with good vertical values.
 if (all(zin == missing_r8)) then
-   istatus(:) = 0
+   istatus(:) = 0 ! vertisundef might come here - nsc  82    ! FIXME: Ha - Why should this be zero? I created 82 if all zin are missing.


same comment as before. if the location uses VERTISUNDEF as the type it often uses missing_r8 as the vertical value. setting this to an error code would fail the vertical conversion of all those obs. in this case, the code is right but clearly the comments need to be more explicit about mentioning VERTISUNDEF after line 4597 and then removing the comments on 4598.

nancycollins · 2020-12-11T20:55:02Z

models/mpas_atm/model_mod.f90

@@ -4644,7 +4708,7 @@ subroutine convert_vert_distrib(state_handle, ens_size, location, obs_kind, ztyp
   enddo

   if (debug > 9 .and. do_output()) then
-      write(string2,'("zout_in_height [m]:",F10.2)') zout
+      write(string2,'("zout_in_height [m]:",F10.2)') zout(1)


if someone does change this code to print all the zout() values, it has to go into a loop. 80 or 100 member ensemble values won't fit into our strings. tests with 2-3 ensemble members work, then full-up experiments fail by overwriting the string. i think in most cases a single sample is enough to see if there's a gross problem - and the user can edit the code to loop over all the members to drill down. so i'd argue leaving the zout(1) for now.

models/mpas_atm/model_mod.f90

nancycollins · 2020-12-11T21:08:01Z

observations/forward_operators/obs_def_rttov_mod.f90


 ! set the surface fields
-call interpolate(state_handle, ens_size, loc_sfc, QTY_SURFACE_PRESSURE, atmos%sfc_p(:), this_istatus)
-call check_status('QTY_SURFACE_PRESSURE', ens_size, this_istatus, val, loc_sfc, istatus, routine, source, revision, revdate, .true., return_now)
+! start with elevation so the other locations can have appropriate vert values


notes for future use: here is where this code could be getting the elevation from some source other than the model - some independent elevation map. but for now, to avoid all these obs being rejected, the easy way is to ask the model for the (model) height at this location, and use it below. that guarantees it won't be rejected by height-mismatch tests, but also prevents it from being legitimately rejected if the actual earth height at this location is not represented well by the model. [ not sure where you're keeping track of future revisions to this code ]

live review with Tim going

For meta data increase have one process not all processes print

hkershaw-brown added 11 commits December 2, 2020 17:14

Add check for equal pressures

1de2f25

Check "pressure is monotonically increasing from TOA down" requires => rather than just >.

Change E_MSG to E_ALLMSG for debugging/error catching calls

c8b2d67

Correct greater than or equal to >=

bb05570

Set surface elevations for atmosphere profiles at 0m 2m 10m

0a1a4be

Using the model QTY_SURFACE_ELEVATION

Initial mpas_atm changes for rttov from Nancy and Soyoung

ae4eec2

The mpas-rttov debugging is descriped in this issue: NCAR/DART_development#119

lowval and uppval no need to be ens size

81c79e2

correct loop naming for members loop

Check for non zero istatus entering theta_to_tk

a59d89a

Pass values(1,:) when compute_scalar_with_barycentric is called with n=1

9ec641b

Discussion here: NCAR/DART_development#119

Removed unsed variable. Changed debugging if value location to match

2a5857b

the comment on debugging info

Add debug > 0 to theta_to_rk message

97e3784

Correction: Add debug > 0 to theta_to_rk message

89606e1

hkershaw-brown requested a review from nancycollins December 11, 2020 18:13

timhoar reviewed Dec 11, 2020

View reviewed changes

Removed commented out code and Ha except for FIXME comments

a7ef51a

nancycollins reviewed Dec 11, 2020

View reviewed changes

models/mpas_atm/model_mod.f90 Show resolved Hide resolved

nancycollins reviewed Dec 11, 2020

View reviewed changes

models/mpas_atm/model_mod.f90 Outdated Show resolved Hide resolved

nancycollins reviewed Dec 11, 2020

View reviewed changes

models/mpas_atm/model_mod.f90 Show resolved Hide resolved

hkershaw-brown added 2 commits December 14, 2020 08:52

Increase debug level back to previous levels for model_interpolate tests

2230ae7

Removed istatus 82

9a8ca14

VERTISUNDEF locations often come in with missing_r8 as the vertical Note there is an early return in vert_convert_distrib_state when VERTISUNDEF so I am not sure which errors the istatus 82 was catching

Debug message for member 1 print

d0d6342

hkershaw-brown mentioned this pull request Dec 14, 2020

mpas elevation check #96

Closed

nancycollins reviewed Dec 15, 2020

View reviewed changes

hkershaw-brown and others added 4 commits December 15, 2020 13:53

Update model_mod.f90

789af2c

live review with Tim going

Update obs_def_rttov_mod.f90

3f4613d

For meta data increase have one process not all processes print

Merge remote-tracking branch 'upstream/master' into bugfix-mpas-rttov

87f5b7d

Clarifying why we are adding the surface elevation to sfc locations

c315287

hkershaw-brown merged commit b9eb67f into master Dec 18, 2020

hkershaw-brown mentioned this pull request Dec 18, 2020

mpas VERTISURFACE change #80

Closed

hkershaw-brown deleted the bugfix-mpas-rttov branch December 18, 2020 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mpas rttov #95

mpas rttov #95

hkershaw-brown commented Dec 11, 2020

timhoar left a comment

timhoar commented Dec 14, 2020

nancycollins Dec 11, 2020

nancycollins Dec 11, 2020

nancycollins Dec 11, 2020

nancycollins Dec 11, 2020

mpas rttov #95

mpas rttov #95

Conversation

hkershaw-brown commented Dec 11, 2020

timhoar left a comment

Choose a reason for hiding this comment

timhoar commented Dec 14, 2020

nancycollins Dec 11, 2020

Choose a reason for hiding this comment

nancycollins Dec 11, 2020

Choose a reason for hiding this comment

nancycollins Dec 11, 2020

Choose a reason for hiding this comment

nancycollins Dec 11, 2020

Choose a reason for hiding this comment