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

Add 'netcdf' and 'grib2' to input 'source' options in regional IC/LBC routines #96

Merged
merged 10 commits into from
May 7, 2021
Merged

Add 'netcdf' and 'grib2' to input 'source' options in regional IC/LBC routines #96

merged 10 commits into from
May 7, 2021

Conversation

chan-hoo
Copy link

@chan-hoo chan-hoo commented Apr 15, 2021

Description

  • Add 'netcdf' and 'grib2' to the input 'source' option in the regional IC/LBC routines.
  • Since the logic of 'netcdf' or 'grib2' in the code is the same as that of 'nemsio', these three types are put in the same category.
  • The data source strings including the old one for 'nemsio' is changed to the logical flag 'data_source_fv3gfs'.

Fixes #97

How Has This Been Tested?

  • Passed the regression test of the ufs weather model on WCOSS (dell/cray) and Hera with intel compiler.
  • Completed a LAMDA test successfully with 'netcdf' input files from GFS.

Checklist:
Please check all whether they apply or not

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • [N/A] Any dependent changes have been merged and published in downstream modules

Contributors
@JamesAbeles-NOAA, @EricRogers-NOAA

Copy link

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I count 11 (!) matches of 'FV3GFS NEMSIO/NETCDF/GRIB2 FILE' in the changes shown here. This clearly warrants introducing a string such as

character(len=*), parameter :: data_source_fv3gfs = 'FV3GFS NEMSIO/NETCDF/GRIB2 FILE'

and then test against it.

@lharris4
Copy link
Contributor

Where is this string being read from? If it is read directly from the file, how would this handle older files that still just say "FV3GFS GAUSSIAN NEMSIO FILE"?

My recommendation is to do this instead:

  1. Read the string when opening the file.
  2. if it matches any of the available strings indicating that this is from FV3GFS, set a logical flag indicating that to be the case.
  3. Check the flag and not a string at the appropriate parts of the code.

This is a best short-term fix. Long-term, we should have attributes in the variables indicating which kind they are (hydrostatic vs. nonhydrostatic height, moist mass vs. total mass defined pressure and tracers, etc.) and then check those individually.

@chan-hoo
Copy link
Author

@lharris4, @climbfuji

In subroutine get_data_source (ln 6647)

if ( trim(source)=='FV3GFS GAUSSIAN NEMSIO FILE' .or. &
trim(source)=='FV3GFS GAUSSIAN NETCDF FILE' .or. &
trim(source)=='FV3GFS GRIB2 FILE' ) then
source = 'FV3GFS NEMSIO/NETCDF/GRIB2 FILE'
if (mpp_pe()==0) write(,) 'New IC source name=',source
endif

In any cases of nemsio, netcdf, grib2, the source name is changed to 'FV3GFS NEMSIO/NETCDF/GRIB2'.
Do you still think that a logical flag is necessary?

@junwang-noaa
Copy link
Collaborator

@chan-hoo I assume this will fix the ufs-weather-model issue#97, right?

@chan-hoo
Copy link
Author

chan-hoo commented Apr 15, 2021

@junwang-noaa, yes. (Jeff said 'not completely', sorry)

@chan-hoo
Copy link
Author

@lharris4, @climbfuji,
How about using numbers rather than a logical flag? Those three are for gfs inputs. If there is a plan to extend other input sources, this approach will be more extendable. For example, gfs nemsio, grib2, netcdf => src_grp=1, hrrr=> src_grp=2, rap=> src_grp=3 ... something like this.

@JeffBeck-NOAA
Copy link

I want to thank @chan-hoo for putting this PR together. It represents a step in the right direction to allow all tracers from different file types (netcdf, grib2) to be used at initialization and not ignored. This was the original problem that we identified, where hydrometeor fields from grib2 source data were being incorrectly thrown out before initialization. However, this is unfortunately not a complete fix for issue #97. The mass and temperature modifications, using w or omega, and other adjustments that are based on this string require more granularity (i.e., whether the source data are FV3GFS, spectral GFS, HRRR, RAP, NAM, UKMET, etc.). This is what @LarissaReames-NOAA and I were discussing in an email thread with @JacobCarley-NOAA. This final step would require understanding each adjustment made in these read routines to correctly apply them to the right external model data. Does anyone know who wrote these read routines? Once we have an idea for each adjustment that is being made, we can identify the external model source data that they correspond to, and introduce extra source strings that would be correctly applied.

@JacobCarley-NOAA
Copy link

@JeffBeck-NOAA This came up in an internal meeting at EMC earlier today and the logic in the code dates back to the earlier days of spinning up the LAM effort. Jim Abeles is familiar with the codes in question. I tried tagging him here but couldn't find his github id.

@JeffBeck-NOAA
Copy link

Thank you, @JacobCarley-NOAA! I believe you can still manually tag people when GitHub can't find them, so I will tag @JamesAbeles-NOAA here.

@JamesAbeles-NOAA
Copy link

JamesAbeles-NOAA commented Apr 15, 2021 via email

@lharris4
Copy link
Contributor

lharris4 commented Apr 15, 2021 via email

@chan-hoo
Copy link
Author

@lharris4, No. The original change is NOT incorrect. As I mentioned above, the old string is included in the change as below:

if ( trim(source)=='FV3GFS GAUSSIAN NEMSIO FILE' .or. &
trim(source)=='FV3GFS GAUSSIAN NETCDF FILE' .or. &
trim(source)=='FV3GFS GRIB2 FILE' ) then
source = 'FV3GFS NEMSIO/NETCDF/GRIB2 FILE'
if (mpp_pe()==0) write(,) 'New IC source name=',source
endif

The old string will be acceptable in the code.

@chan-hoo
Copy link
Author

I've changed the source strings to source groups.

@bensonr
Copy link
Contributor

bensonr commented Apr 22, 2021

A meeting was held regarding this topic and it was decided the proper approach is to have the IC/LBC preprocessing tool chgres_cube provide the data in the expected formats given the numerous sources chgres_cube can ingest. The meeting notes have been captured in an internal document.

Closing this PR for now.

@JacobCarley-NOAA
Copy link

Can this be re-opened? @chan-hoo 's changes are a critical need now and while there are plans in place to address the issue in a more all encompassing manner I don't think it's a problem for us to move forward with this PR.

@bensonr bensonr reopened this Apr 28, 2021
@bensonr
Copy link
Contributor

bensonr commented Apr 28, 2021

@JacobCarley-NOAA Please open an issue assigned to yourself as a reminder to clean this all back up once the proper fix is in place.

@chan-hoo
Copy link
Author

@bensonr, thank you for reopening this pr. @junwang-noaa, @DusanJovic-NOAA, please review this pr again. LAM/LAM-DA people need this change to run their experiments without failure.

@bensonr
Copy link
Contributor

bensonr commented May 3, 2021

@DusanJovic-NOAA @junwang-noaa @climbfuji - can you perform another review?

@bensonr
Copy link
Contributor

bensonr commented May 4, 2021

@chan-hoo if you have addressed all of the comments, can you please resolve the conversations here and I will merge it tomorrow.

@chan-hoo
Copy link
Author

chan-hoo commented May 4, 2021

@bensonr, all the comments have been resolved. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants