bug-fix: get_dist calculation for observations with VERTISUNDEF in WRF

[hkershaw-brown]

Description:

Previously observations with VERTISUNDEF were given a large distance, because the vertical coordinate is missing: local_array(3) == missing_r8

DART/models/wrf/model_mod.f90

Lines 6423 to 6424 in 1b76f3a

	if (((vertical_localization_on()).and.(local_array(3) == missing_r8)).or.(istatus2 == 1)) then
	dist(k) = 1.0e9

The location_mod::get_dist uses horizontal distance only if VERTISUNDEF for either location.

Fixes issue

Fixes #486

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Documentation changes needed?

• My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Tests

Please describe any tests you ran to verify your changes.
Tested using an example case from Lukas K.
#486 and main...lukas-vertundef-wrf
Note lukas-vertundef-wrf has changes for wrf v4, this pull request only has the VERTISUNDEF bug fix.

Checklist for merging

• Updated changelog entry
• Documentation updated
• Update conf.py

Checklist for release

• Merge into main
• Create release from the main branch with appropriate tag
• Delete feature-branch

Testing Datasets

• Dataset needed for testing available upon request
• Dataset download instructions included
• No dataset needed

[mjs2369]

@hkershaw-brown the general fix here looks great, which adds the check for VERTISUNDEF obs that calculates the 2D dist before converting the local vert coordinate.

I have a few logistical questions:

The first is about this comment ( #486 (comment) ) where you explain why you changed the value of dist for testing. Should we maybe keep this change, even outside of testing, so that we have a different value for initialization and missing/failed vert coordinates?

And the second is just revisiting a question that you initially asked in the notes of the issue, which is whether or not we should set whichvert_obs_in_localization_coord = 0 here -

DART/assimilation_code/modules/assimilation/assim_tools_mod.f90

Line 593 in 72e5740

whichvert_obs_in_localization_coord = 0

As you noted it doesn't match any of the values in the 3D sphere location_mod:
should this be -2 to match the VERTISUNDEF parameter value?

DART/assimilation_code/location/threed_sphere/location_mod.f90

Line 62 in 72e5740

integer, parameter :: VERTISUNDEF = -2 ! has no specific vertical location (undefined)

The ability to do this is dependent on whether or not setting the value to -2 will break the other location mods other than 3D sphere, but it's hard to tell because most location mods either don't have this set of parameters or only use a select number of them (like here in 3D cartesian)

DART/assimilation_code/location/threed_cartesian/location_mod.f90

Line 50 in 1b76f3a

integer, parameter :: VERTISHEIGHT = 1 ! by height (in meters)

On the flip side, could we potentially change the parameter value for VERTISUNDEF to 0 for in threed_sphere/location_mod.f90?

[hkershaw-brown]

The first is about this comment ( #486 (comment) ) where you explain why you changed the value of dist for testing. Should we maybe keep this change, even outside of testing, so that we have a different value for initialization and missing/failed vert coordinates?

I thought about this too. I was using a debugger to trap on various values, which i think nice debugging technique, but
I think if you start putting extra information into the distance value, e.g.
dist (output from get_close) is both and distance and an indication of why the distance is the value it is,
you're breaking the contract between get_close and the code that calls get_close.

Note there is an upcoming project to simplify a lot of the wrf model_mod #404 and part of that is to not have so many if statements in get_close.

whichvert_obs_in_localization_coord = 0

VERTISUNDEF is in a world where the vertical dimension exists, but the observation is not at one point in the vertical (e.g. maybe it is the integral of a whole column of atmosphere)
vs. in a one dimensional or two dimensional world (e.g. flatland) where there is no concept of the vertical dimension.

I think whichvert_obs_in_localization_coord is just set to something because it is broadcast (the task that owns an observation does vertical conversion and broadcasts the result out along with all the other stuff that task calculates).

DART/assimilation_code/modules/assimilation/assim_tools_mod.f90

Lines 613 to 617 in 1b76f3a

	whichvert_real = real(whichvert_obs_in_localization_coord, r8)
	call broadcast_send(map_pe_to_task(ens_handle, owner), obs_prior, &
	orig_obs_prior_mean, orig_obs_prior_var, &
	scalar1=obs_qc, scalar2=vertvalue_obs_in_localization_coord, &
	scalar3=whichvert_real, scalar4=my_inflate, scalar5=my_inflate_sd)

but whichvert_obs_in_localization_coord is never used if the model_mod does not a have a vertical dimension:

DART/assimilation_code/modules/assimilation/assim_tools_mod.f90

Line 293 in 1b76f3a

is_doing_vertical_conversion = (has_vertical_choice() .and. vertical_localization_on())

I'm guessing that 0 was picked because it is not a valid value for the vertical so if you start to use it, it should trigger an error.

I don't think whichvert_obs_in_localization_coord is great for readability in assim_tools_mod (since it confused me! hehehe). But I'd push any refactoring of it into the future when we have a good look at assim_tool_mod.

[mjs2369]

Sounds good. Like you said, we can revisit any of these issues when we refactor. Approved.