Skip to content

fix: vtec extrapolation calculation in TIEGCM #852

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

Merged
merged 2 commits into from
May 1, 2025
Merged

Conversation

hkershaw-brown
Copy link
Member

@hkershaw-brown hkershaw-brown commented Apr 1, 2025

Description:

Fixes problems in model_interpolate for VTEC

  • result of 4 corners was not being stored correctly, overwriting corner11
  • domain of ZG was incorrect
  • units of ZG for need to be m in the vtec calculation

I've taken out the todo "test vtec" 🙃

Fixes issue

fixes #851

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

None yet, just compiled user reported changes.
Derecho is down today.

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

@hkershaw-brown hkershaw-brown added the tiegcm Thermosphere-Ionosphere-Electrodynamics General Circulation Model label Apr 3, 2025
@hkershaw-brown hkershaw-brown requested a review from mjs2369 April 22, 2025 19:53
Copy link
Contributor

@mjs2369 mjs2369 left a comment

Choose a reason for hiding this comment

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

Only one question, a little unrelated to the PR, but concerns val11, 12, 21, 22

bogus_level = 1 !HK what should this be? Do only 2D fields have VERTISUNDEF?

Looks like you made this comment 4 years ago and I share the same concern - if 1 is a valid value for level, then is 1 a good value for a bogus_level? It doesn't look like it really matters, since any 2D variables (that therefore have VERTISUNDEF) are not using the value for level at all when it is passed into get_dart_vector_index (note that level becomes kloc)

if (ndims == 1) then
get_dart_vector_index = offset + iloc - 1
else if(ndims == 2) then
get_dart_vector_index = offset + iloc + (jloc-1)*dsize(1) - 1
else if(ndims == 3) then
get_dart_vector_index = offset + iloc + (jloc-1)*dsize(1) + (kloc-1)*dsize(1)*dsize(2) - 1

But just something to think about. Feel free to just tell me to disregard this - other than this I see no issues with the changes here so I'll go ahead and approve

@hkershaw-brown
Copy link
Member Author

Only one question, a little unrelated to the PR, but concerns val11, 12, 21, 22

bogus_level = 1 !HK what should this be? Do only 2D fields have VERTISUNDEF?

Looks like you made this comment 4 years ago and I share the same concern - if 1 is a valid value for level, then is 1 a good value for a bogus_level? It doesn't look like it really matters, since any 2D variables (that therefore have VERTISUNDEF) are not using the value for level at all when it is passed into get_dart_vector_index (note that level becomes kloc)

if (ndims == 1) then
get_dart_vector_index = offset + iloc - 1
else if(ndims == 2) then
get_dart_vector_index = offset + iloc + (jloc-1)*dsize(1) - 1
else if(ndims == 3) then
get_dart_vector_index = offset + iloc + (jloc-1)*dsize(1) + (kloc-1)*dsize(1)*dsize(2) - 1

But just something to think about. Feel free to just tell me to disregard this - other than this I see no issues with the changes here so I'll go ahead and approve

Its hanging out from this comment:

! @todo from Lanai code:
! FIXME ... is it possible to try to get a pressure with which_vert == undefined
! At present, vert_interp will simply fail because height is a negative number.
! @todo HK what are you supposed to do for pressure with VERTISUNDEF? level 1?

I am not sure what the correct thing to do is still, or if there are calls to model_interpolate with VERTISUNDEF.

@hkershaw-brown hkershaw-brown added the release! bundle with next release label Apr 29, 2025
@mjs2369
Copy link
Contributor

mjs2369 commented Apr 29, 2025

Its hanging out from this comment:

! @todo from Lanai code:
! FIXME ... is it possible to try to get a pressure with which_vert == undefined
! At present, vert_interp will simply fail because height is a negative number.
! @todo HK what are you supposed to do for pressure with VERTISUNDEF? level 1?

I am not sure what the correct thing to do is still, or if there are calls to model_interpolate with VERTISUNDEF.

Let's just punt on this for now then, the rest of the PR looks great

@nsc1
Copy link

nsc1 commented Apr 29, 2025

my guess is that the level=1 code was put in before the state structure was used, and all three indices were used to compute the location in the state vector. it may not matter anymore because the state structure code knows about 2d fields.

also, the answer to the question on line 419:
! HK why does it have to be QTY_GEOMETRIC_HEIGHT?

it doesn't - but we had usually used something that was simple/fast to interpolate so the code didn't go through a long computation to find a value that wasn't going to be used other than a yes/no counter.

- result of 4 corners was not being stored correctly, overwritting corner11
- domain of ZG was incorrect
- units of ZG for need to be m in the vtec calculation

fixes #851
@hkershaw-brown hkershaw-brown merged commit 7c22a86 into main May 1, 2025
4 checks passed
@hkershaw-brown hkershaw-brown deleted the fix-vtec branch May 1, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release! bundle with next release tiegcm Thermosphere-Ionosphere-Electrodynamics General Circulation Model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: TIEGCM vtec calulcation
3 participants