Feature 1815 level value #2190

hsoh-u · 2022-06-15T20:03:57Z

Expected Differences

No differences except suppoort @NNN for vertical level and time.
New output (plot_data_plane/gtg_obs_forecast.20130730.i00.f00.NCCF_latlon_20000.ps) is added to unit test.

Do these changes introduce new tools, command line arguments, or configuration file options? [No]

If yes, please describe:

No, it's a change to the configuration.
Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]

If yes, please describe:

Pull Request Testing

Describe testing already performed for these changes:

Input file with "axis"=Z

#using z=20000 (flight level in feet)
/d1/personal/hsoh/git/features/feature_1815_level_value/MET/bin/plot_data_plane /d1/projects/MET/MET_test_data/unit_test/model_data/nccf/gtg/latlon/gtg_obs_forecast.20130730.i00.f00.nc slice_pressure_value_20000_1.ps 'name = "edr"; level = "(0,20000,*,*)";' -title "NCCF Latitude/Longitude Level 0" -v 7

#using integer value @20000 (flight level in feet)
/d1/personal/hsoh/git/features/feature_1815_level_value/MET/bin/plot_data_plane /d1/projects/MET/MET_test_data/unit_test/model_data/nccf/gtg/latlon/gtg_obs_forecast.20130730.i00.f00.nc slice_pressure_value_20000_2.ps 'name = "edr"; level = "(0,@20000,*,*)";' -title "NCCF Latitude/Longitude Level 0" -v 7

#using float value @20000.0 (flight level in feet)
/d1/personal/hsoh/git/features/feature_1815_level_value/MET/bin/plot_data_plane /d1/projects/MET/MET_test_data/unit_test/model_data/nccf/gtg/latlon/gtg_obs_forecast.20130730.i00.f00.nc slice_pressure_value_20000f.ps 'name = "edr"; level = "(0,@20000.0,*,*)";' -title "NCCF Latitude/Longitude Level 0" -v 7

Input file without "axis"=Z, but with the same variable name with the dimension name for the vertical level ("lev")

#using float value @@7.59481964632869
/d1/personal/hsoh/git/features/feature_1815_level_value/MET/bin/plot_data_plane /d1/projects/MET/MET_test_data/unit_test/model_data/easm/b.e15.B1850.f09_g16.pi_control.all_ga7cpl24.66.cam.i.0083-01-01-00000.nc same_dim_var_name.ps 'name = "T"; level = "(0,@7.59481964632869,*,*)";' -title "NCCF Latitude/Longitude Level 0" -v 7

Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Do these changes include sufficient testing updates? [Yes]
Will this PR result in changes to the test suite? [Yes]

If yes, describe the new output and/or changes to the existing output:

New unit test is added (plot_data_plane/gtg_obs_forecast.20130730.i00.f00.NCCF_latlon_20000.ps).
YYYYMMDD_HHMMDD is changed to @YYYYMMDD_HHMMDD at unit test definition file

Please complete this pull request review by [Fill in date].

Pull Request Checklist

See the METplus Workflow for details.

Review the source issue metadata (required labels, projects, and milestone).
Complete the PR definition above.
Ensure the PR title matches the feature or bugfix branch name.
Define the PR metadata, as permissions allow.
Select: Reviewer(s)
Select: Organization level software support Project or Repository level development cycle Project
Select: Milestone as the version that will include these changes
After submitting the PR, select Linked issues with the original issue number.
After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
Close the linked issue and delete your feature or bugfix branch from GitHub.

…cur_time_index

…cur_time_index. Support value instead of offset for time and z level

…_value

JohnHalleyGotway

@hsoh-u I ran into an issue testing output pinterp NetCDF files.

ncdump -h $MET_TEST_INPUT/model_data/p_interp/wrfout_d01_2008-08-08_12\:00\:00_PLEV
...
        float pressure(num_metgrid_levels) ;
                pressure:long_name = "Pressure Levels" ;
                pressure:standard_name = "air_pressure" ;
                pressure:units = "hPa" ;
                pressure:positive = "down" ;
...
        float RH(Time, num_metgrid_levels, south_north, west_east) ;
                RH:FieldType = 104 ;
                RH:MemoryOrder = "XZY" ;
                RH:description = "Relative Humidity" ;
                RH:units = "%" ;
                RH:stagger = "-" ;
                RH:coordinates = "XLONG XLAT" ;

And I ran:

plot_data_plane \
/Volumes/d1/projects/MET/MET_unit_test/MET_test_input/model_data/p_interp/wrfout_d01_2008-08-08_12\:00\:00_PLEV \
plot.ps 'name="RH"; level="(@20080808_120000,@750,*,*)";' -v 7

Note that this should NOT actually work since "num_metgrid_levels" is NOT a coordinate variable. But requesting @750 (or really any other value) for the 2nd dim always results in index 0 being used for that dimension.

Could you please check the logic of this?

hsoh-u · 2022-06-22T17:21:03Z

Only two cases are supported:

has a variable with "axis" attribute with "Z"
has the variable with the same dimension name for the vertical level

Above case is not supported yet but it's doable with supporting two cases:

by looking at the standard_name: "air_pressure" and "height" (can be extended with more pre-defined variable names)
Find the the variable with the same dimension for the vertical level and use it only if one variable exists with the same dimension

        num_metgrid_levels = 38 ;
        float pressure(num_metgrid_levels) ;
        float RH(Time, num_metgrid_levels, south_north, west_east) ;

…_value

hsoh-u · 2022-06-30T14:51:58Z

In case of $MET_TEST_INPUT/model_data/p_interp/wrfout_d01_2008-08-08_12:00:00_PLEV, MET is looking for the variable "num_metgrid_levels" and "Time" (which is the same name of the dimension). "Time" variable is selected for time dimension. If not exists, MET is looking for the the first 1 dimension variable which has the dimension, "num_metgrid_levels". The "pressure" variable is selected for the vertical level. MET does not check if there two or more variables exist with the same condition.

        double Time(Time) ;
                Time:long_name = "Time" ;
                Time:units = "hours since 1997-01-01 00:00:00" ;
                Time:calendar = "standard" ;
        float pressure(num_metgrid_levels) ;
                pressure:long_name = "Pressure Levels" ;
                pressure:standard_name = "air_pressure" ;
                pressure:units = "hPa" ;
                pressure:positive = "down" ;
        float RH(Time, num_metgrid_levels, south_north, west_east) ;
                RH:FieldType = 104 ;
                RH:MemoryOrder = "XZY" ;
                RH:description = "Relative Humidity" ;
                RH:units = "%" ;
                RH:stagger = "-" ;
                RH:coordinates = "XLONG XLAT" ;

JohnHalleyGotway · 2022-06-30T16:24:23Z

Reassigning the review of this PR from @JohnHalleyGotway to @georgemccabe, since I'm unavailable today.

Thanks @georgemccabe for taking a look at it. Please see the issues I ran into listed in this comment. @hsoh-u has made changes to fix them.

georgemccabe · 2022-06-30T17:15:50Z

internal/test_unit/xml/unit_plot_data_plane.xml

@@ -292,7 +306,7 @@
    <param> \
      &DATA_DIR_MODEL;/nccf/GloTEC_TEC_2015_03_17.nc \
      &OUTPUT_DIR;/plot_data_plane/plot_data_plane_NCCF_time.ps \
-      'name="TEC"; level="(20150317_000500,*,*)"; file_type=NETCDF_NCCF;' \
+      'name="TEC"; level="(@20150317_000500,*,*)"; file_type=NETCDF_NCCF;' \


Do we still have tests that ensure that specifying the time value still works without the added @ symbol? If not, I think we should add at least 1 since that syntax is currently used by users.

georgemccabe

I reviewed the code changes. There are a lot of them! I didn't examine every change in detail, but they seemed to make sense and I didn't notice any obvious mistakes or bugs. I ran a few tests using different input files and verified that the expected logic appears to work properly. This is a great enhancement!

I made one in-line comment about making sure that there is still at least 1 unit test that ensures that the date string can be specified without the @ symbol prepended since that syntax is currently used by users. If there are no longer any tests that demonstrate this, I suggest that one is added. Otherwise, I approve.

I see that the automated tests flagged a difference due to new output from the new unit test. Once this PR is merged, the develop-ref branch should be updated to regenerate the truth dataset.

hsoh-u · 2022-06-30T20:32:49Z

Updated and 3 test cases exist:

@vertical_value
@time
time stirng without @

      'name = "edr"; level = "(0,@20000,*,*)";' \
      'name="TEC"; level="(20150317_000500,*,*)"; file_type=NETCDF_NCCF;' \
      'name="pr"; level="(@20060102_000000,*,*)";' \

georgemccabe

Looks great, thanks! I approve!
See previous review for details.

Howard Soh added 15 commits June 13, 2022 17:44

#1815 Renamed time_as_offset to Is_offset

7ea50d0

#1815 Added vx_data2d_dim_by_value

c5e9639

#1815 Added convert_value_to_offset and renamed _time_dim_offset to _…

e7152fa

…cur_time_index

#1815 Added convert_value_to_offset and renamed _time_dim_offset to _…

2d0ff3a

…cur_time_index. Support value instead of offset for time and z level

#1815 Support @ for the value insteaed of offset for slicing

80dd0a8

#1815 Support value insteaed of offset for slicing

fc5a5cb

#1815 Deleted unused local variable

8d59e18

#1815 Deleted unused local variable

7837db8

#1815 Added level by value with @

239dd08

#1815 Added plot_data_plane_NCCF_latlon_by_value

7657431

#1815 Added level by value with @

7bce984

#1815 Updated level

329e4f6

Merge remote-tracking branch 'origin/develop' into feature_1815_level…

2c9d528

…_value

#1815 call is_eq to comapre two double values

c8f1f95

Merge remote-tracking branch 'origin/develop' into feature_1815_level…

560c8b7

…_value

JohnHalleyGotway added this to the MET 11.0.0 milestone Jun 22, 2022

JohnHalleyGotway linked an issue Jun 22, 2022 that may be closed by this pull request

Enhance MET's NetCDF library interface to support level strings that include coordinate variable values instead of just indexes. #1815

Closed

20 tasks

JohnHalleyGotway self-requested a review June 22, 2022 16:25

JohnHalleyGotway requested changes Jun 22, 2022

View reviewed changes

Howard Soh added 10 commits June 22, 2022 14:31

Merge remote-tracking branch 'origin/develop' into feature_1815_level…

49ba0ad

…_value

#1815 Added more attribute names

c56a507

#1815 Added get_index_for_dim

ca77ab2

#1815 Formatting

9483883

#1815 Support a differnt dimension name from thw first data variable

6741041

#1815 Use constants for attr names

033ccda

#1815 cleanup

d4886d3

#1815 cleanup

5a1e381

#1851 cleanup

1a9f1b4

#1851 cleanup

38211de

Howard Soh added 7 commits June 27, 2022 12:15

#1815 Added get_att_unixtime

1f63628

#1851 cleanup

7c1ad06

#1815 cleanup

ca162f5

#1815 cleanup

4f1343f

#1815 Support @ for pinterp

9541e84

#1815 Do not call to_string

2066939

#1815 cleanup

161227f

hsoh-u requested a review from JohnHalleyGotway June 30, 2022 14:46

JohnHalleyGotway requested review from georgemccabe and JohnHalleyGotway and removed request for JohnHalleyGotway June 30, 2022 16:22

JohnHalleyGotway requested review from JohnHalleyGotway and removed request for JohnHalleyGotway June 30, 2022 16:24

georgemccabe reviewed Jun 30, 2022

View reviewed changes

georgemccabe approved these changes Jun 30, 2022

View reviewed changes

#1815 Changed @time to time at plot_data_plane_NCCF_time

f4d9706

hsoh-u requested a review from georgemccabe June 30, 2022 20:29

georgemccabe approved these changes Jun 30, 2022

View reviewed changes

hsoh-u merged commit 14a059e into develop Jun 30, 2022

hsoh-u changed the title ~~Feature 1815 level value~~ Feature #1815 level value Jun 30, 2022

hsoh-u changed the title ~~Feature #1815 level value~~ Feature 1815 level value Jun 30, 2022

georgemccabe mentioned this pull request Jun 30, 2022

Revert "Feature 1815 level value" #2201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature 1815 level value #2190

Feature 1815 level value #2190

hsoh-u commented Jun 15, 2022 •

edited

Loading

JohnHalleyGotway left a comment •

edited

Loading

hsoh-u commented Jun 22, 2022

hsoh-u commented Jun 30, 2022 •

edited

Loading

JohnHalleyGotway commented Jun 30, 2022

georgemccabe Jun 30, 2022

georgemccabe left a comment

hsoh-u commented Jun 30, 2022

georgemccabe left a comment

Feature 1815 level value #2190

Feature 1815 level value #2190

Conversation

hsoh-u commented Jun 15, 2022 • edited Loading

Expected Differences

Pull Request Testing

Pull Request Checklist

JohnHalleyGotway left a comment • edited Loading

Choose a reason for hiding this comment

hsoh-u commented Jun 22, 2022

hsoh-u commented Jun 30, 2022 • edited Loading

JohnHalleyGotway commented Jun 30, 2022

georgemccabe Jun 30, 2022

Choose a reason for hiding this comment

georgemccabe left a comment

Choose a reason for hiding this comment

hsoh-u commented Jun 30, 2022

georgemccabe left a comment

Choose a reason for hiding this comment

hsoh-u commented Jun 15, 2022 •

edited

Loading

JohnHalleyGotway left a comment •

edited

Loading

hsoh-u commented Jun 30, 2022 •

edited

Loading