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

Improve moving nest land mask handling #772

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

wramstrom
Copy link
Contributor

@wramstrom wramstrom commented Jan 19, 2024

Description

Improves handling of land masked variables in moving nest code by two means. First, interpolation of variables near coastlines had some errors corrected. Second, the static surface variables (including land mask) are populated into the high-resolution static surface variables for the whole parent panel. This eliminates occasional inconsistencies that occurred when land fraction was very close to 0.5. The preprocessing code seems to generate lat/lon points that are a few meters apart on the nest compared to the high resolution parent grid.

Issue(s) addressed

Testing

How were these changes tested?
What compilers / HPCs was it tested with?
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
Have the ufs-weather-model regression test been run? On what platform?

  • Will the code updates change regression test baseline? If yes, why? Please show the baseline directory below.
  • Please commit the regression test log files in your ufs-weather-model branch

Some HAFS moving nest related regression tests may have different results. Other regression tests should be identical.

Dependencies

@BinLiu-NOAA BinLiu-NOAA changed the title Improve moving nest land mask handling. Improve moving nest land mask handling Jan 22, 2024
@BinLiu-NOAA BinLiu-NOAA added ready for review enhancement New feature or request labels Jan 25, 2024



Copy link
Collaborator

@BinLiu-NOAA BinLiu-NOAA Jan 25, 2024

Choose a reason for hiding this comment

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

@wramstrom, in general, coding style wise, I would suggest avoid using more than 2 empty lines (unless absolutely needed) between code blocks.

There are also other locations in this set of changes with more than 2 empty lines if you would like to help to update.

@BinLiu-NOAA
Copy link
Collaborator

@BijuThomas-NOAA, could you please help to build this feature/moving_nest_land_mask branch by turning on all warnings to check if any compiling warning messages will be added with this set of changes. Meanwhile, appreciate it if you could also help to try with the debug and check all (or proper check) building options to make sure it works properly. These will be needed/required by HAFSv2 code delivery. Thanks!

Copy link
Collaborator

@DusanJovic-NOAA DusanJovic-NOAA left a comment

Choose a reason for hiding this comment

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

There are a lot of changes in this PR I'm not familiar with. I can approve the PR based on the successful regression test results, once all tests are done

@BinLiu-NOAA
Copy link
Collaborator

@wramstrom, is it possible (or how difficulty it might be) to make the newly added features/capabilities namelist option(s) controlled? In such case, if/when desired, the existing method can still be used for the moving-nesting configurations.

The backward compatibility, especially when involving additional input data, would be very useful when testing with previous/existing test cases (which do not have the additional input data saved/available).

@wramstrom
Copy link
Contributor Author

@BinLiu-NOAA Yes, I could add an option for the file reads to the namelist, but perhaps not til Friday when I am back home from AMS.

@BinLiu-NOAA
Copy link
Collaborator

Thanks @wramstrom! I think that will be very useful. Also, given we are very close to HAFSv2 code freeze, we probably leave this set of changes targeting 2024 HAFS real-time parallel experiments (instead). In that case, we will have more time to test these changes for their impacts on model stability and forecast skills (if any). @ZhanZhang-NOAA, what do you think?

@ZhanZhang-NOAA
Copy link
Contributor

@BinLiu-NOAA Yes, agreed. Considering we are approaching HAFSv2 code freeze deadline, we should postpone this PR, and target it for 2024 HAFS real-time parallel experiments.

@BinLiu-NOAA
Copy link
Collaborator

Thanks @ZhanZhang-NOAA! With that, @wramstrom, we probably can convert this PR into a draft PR at this point. Once you make the above-mentioned changes (wrapping up with namelist controlled options), we can then continue this PR. Please feel free to let us know if this sounds reasonable. Thanks!

@BinLiu-NOAA
Copy link
Collaborator

With the communication with with @wramstrom and others, we will not include these changes in HAFSv2 code freeze. @wramstrom, if you want, you can convert this PR into a draft PR at this point. Thanks!

@BinLiu-NOAA BinLiu-NOAA marked this pull request as draft February 1, 2024 18:45
@DusanJovic-NOAA
Copy link
Collaborator

What's the status of this PR? It hasn't been updated for 3 months now. @BinLiu-NOAA

@DusanJovic-NOAA
Copy link
Collaborator

What's the status of this PR? It hasn't been updated for more than 3 months now. @BinLiu-NOAA

@BinLiu-NOAA
Copy link
Collaborator

@DusanJovic-NOAA, Sorry for the late response. Based on my previous communication with @wramstrom, there might be some concerns/issues for the current approach implemented especially when considering moving-nesting restart/warm-start capability. So, we might want to hold this PR work for now. Currently, this is a draft PR. If needed/preferred, we can even close this PR for now, and open a PR in the future. Please feel free to let us know your comments/suggestions/preferences, @wramstrom and @DusanJovic-NOAA. Thanks!

@wramstrom
Copy link
Contributor Author

@DusanJovic-NOAA and @BinLiu-NOAA, I think this PR might be useful in one or more of the realtime parallel experiments this summer. It will improve stability of the code and correctness of a few land surface variables at coastlines. The downside that @BinLiu-NOAA refers to is that it requires changes to the file setup during preprocessing, to create additional symbolic links for parent static data. While it is not significant overhead in terms of processing or disk storage, it does mean that cases need the full workflow to run.

@wramstrom
Copy link
Contributor Author

@AndrewHazelton we might want to include this PR in the real-time parallel this summer.

@DusanJovic-NOAA
Copy link
Collaborator

@DusanJovic-NOAA, Sorry for the late response. Based on my previous communication with @wramstrom, there might be some concerns/issues for the current approach implemented especially when considering moving-nesting restart/warm-start capability. So, we might want to hold this PR work for now. Currently, this is a draft PR. If needed/preferred, we can even close this PR for now, and open a PR in the future. Please feel free to let us know your comments/suggestions/preferences, @wramstrom and @DusanJovic-NOAA. Thanks!

You do not need to close it. I just wanted to see if it's still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve handling of land masked surface parameters in HAFS moving nest
4 participants