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

Bugfix: Fix support for the YYYYMMDD format in NetCDF level timestrings #2482

Closed
6 of 21 tasks
John-Sharples opened this issue Feb 21, 2023 · 3 comments · Fixed by #2483 or #2484
Closed
6 of 21 tasks

Bugfix: Fix support for the YYYYMMDD format in NetCDF level timestrings #2482

John-Sharples opened this issue Feb 21, 2023 · 3 comments · Fixed by #2483 or #2484
Assignees
Labels
MET: Library Code priority: medium Medium Priority reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project requestor: Community General Community type: bug Fix something that is not working

Comments

@John-Sharples
Copy link

John-Sharples commented Feb 21, 2023

I'm currently testing a move from METplus 4.1.3 (MET 10.1.2) to 5.0.0 (11.0.0), and noticed this change in behaviour.

Might be bug, but there's an easy work around, so reporting as a request for enhanced doco. I couldn't see anything in the release notes or any related issues, please close if already documented somewhere.

Describe the Enhancement

With METplus 4.1.3 I can run the following without errors
plot_data_plane some_fcst.nc output.ps 'name="apcp";level="(20230102,*,*)";'

With METplus 5.0.0 I get the following:

WARNING: MetNcCFDataFile::convert_time_to_offset() -> 20230102 does not exist at time variable
WARNING:
ERROR  :
ERROR  : MetNcCFDataFile::data_plane(VarInfo &, DataPlane &) -> the requested time offset -9999 for "apcp" variable is out of range (between 0 and 9).
ERROR  :
ERROR  :

The workaround is to provide the date string with full HHMMSS format. e.g.
plot_data_plane some_fcst.nc output.ps 'name="apcp";level="(20230102_000000,*,*)";'

Note that this also applies to other tools. For example, to avoid similar errors in GridStat
FCST_VAR1_LEVELS = "({valid?fmt=%Y%m%d},*,*)"
Should become
FCST_VAR1_LEVELS = "({valid?fmt=%Y%m%d_%H%M%S},*,*)"

Time Estimate

Not much.

Sub-Issues

Consider breaking the enhancement down into sub-issues.

  • Add a checkbox for each sub-issue here.

Relevant Deadlines

NONE.

Funding Source

NONE.

Define the Metadata

Assignee

  • Select engineer(s) or no engineer required
  • Select scientist(s) or no scientist required

Labels

  • Select component(s)
  • Select priority
  • Select requestor(s)

Projects and Milestone

  • Select Repository and/or Organization level Project(s) or add alert: NEED PROJECT ASSIGNMENT label
  • Select Milestone as the next official version or Future Versions

Define Related Issue(s)

Consider the impact to the other METplus components.

Enhancement Checklist

See the METplus Workflow for details.

  • Complete the issue definition above, including the Time Estimate and Funding Source.
  • Fork this repository or create a branch of develop.
    Branch name: feature_<Issue Number>_<Description>
  • Complete the development and test your changes.
  • Add/update log messages for easier debugging.
  • Add/update unit tests.
  • Add/update documentation.
  • Push local changes to GitHub.
  • Submit a pull request to merge into develop.
    Pull request: feature <Issue Number> <Description>
  • Define the pull request metadata, as permissions allow.
    Select: Reviewer(s) and Development issues
    Select: Repository level development cycle Project for the next official release
    Select: Milestone as the next official version
  • Iterate until the reviewer(s) accept and merge your changes.
  • Delete your fork or branch.
  • Close this issue.
@John-Sharples John-Sharples added alert: NEED ACCOUNT KEY Need to assign an account key to this issue alert: NEED MORE DEFINITION Not yet actionable, additional definition required alert: NEED CYCLE ASSIGNMENT Need to assign to a release development cycle type: enhancement Improve something that it is currently doing labels Feb 21, 2023
@JohnHalleyGotway JohnHalleyGotway transferred this issue from dtcenter/MET Mar 2, 2023
@JohnHalleyGotway JohnHalleyGotway added component: documentation Documentation issue priority: medium Medium Priority requestor: Community General Community and removed alert: NEED MORE DEFINITION Not yet actionable, additional definition required labels Mar 2, 2023
@JohnHalleyGotway JohnHalleyGotway transferred this issue from dtcenter/METplus Mar 3, 2023
@JohnHalleyGotway
Copy link
Collaborator

@John-Sharples thanks for letting us know about this unexpected change in behavior. I suspect this change happened during development for MET version 11.0.0 when we added support for referencing dimensions by their value rather than just by their index (dtcenter/MET#1815).

Our intention has been to support time strings following this convention: YYYYMMDD[_HH[MMSS]], where the square brackets are optional.

So July 30, 2013 could be written as 20130730, 20130730_00, or 20130730_000000. But I just tested the current state of the develop branch and confirmed the behavior you describe. While 20130730_00 and 20130730_000000 are processed as expected, using 20130730 does not:

bin/plot_data_plane /d1/projects/MET/MET_test_data/unit_test/model_data/nccf/gtg/latlon/gtg_obs_forecast.20130730.i00.f00.nc plot.ps 'name="edr"; level="(20130730,0,*,*)";'
WARNING: MetNcCFDataFile::convert_time_to_offset() -> 20130730 does not exist at time variable

While that same command works just fine for MET version 10.0.0:

/usr/local/met-10.1.0/bin/plot_data_plane /d1/projects/MET/MET_test_data/unit_test/model_data/nccf/gtg/latlon/gtg_obs_forecast.20130730.i00.f00.nc plot.ps 'name="edr"; level="(20130730,0,*,*)";'

@JohnHalleyGotway JohnHalleyGotway removed the alert: NEED CYCLE ASSIGNMENT Need to assign to a release development cycle label Mar 3, 2023
@JohnHalleyGotway JohnHalleyGotway moved this from 📋 Backlog to 🔖 Ready in Coordinated METplus-5.0 Support Mar 3, 2023
@JohnHalleyGotway JohnHalleyGotway moved this from 📋 Backlog to 🔖 Ready in MET-11.1.0 Development Mar 3, 2023
@JohnHalleyGotway JohnHalleyGotway added this to the MET 11.0.2 (bugfix) milestone Mar 3, 2023
@JohnHalleyGotway JohnHalleyGotway added type: bug Fix something that is not working and removed type: enhancement Improve something that it is currently doing labels Mar 3, 2023
@JohnHalleyGotway
Copy link
Collaborator

JohnHalleyGotway commented Mar 3, 2023

@hsoh-u, please take a look at this unexpected change in behavior. Let's treat this as a bug and provide a fix for it in both the main_v11.0 and develop branches.

While MET version 10.1.0 supports NetCDF timestrings YYYYMMDD, YYYYMMDD_HH, and YYYYMMDD_HHMMSS formats, MET version 11.0.0 NO LONGER supports the YYYYMMDD format. Dropping this support was unintentional to my knowledge and I'd like to add it back in.

If you're not already doing so, please call the timestring_to_unix() function to parse the timestring. That supports even a wider variety of time formats.

Please also add to the develop branch a unit test to demonstrate using the YYYYMMDD format to ensure that usage continue to work.

@JohnHalleyGotway JohnHalleyGotway changed the title Document changes to date format in METplus 5.0 when using netCDF inputs Bugfix: Fix support for the YYYYMMDD format in NetCDF level timestrings Mar 3, 2023
@hsoh-u
Copy link
Collaborator

hsoh-u commented Mar 3, 2023

It's caused by the order of checking: string as an integer or string as a time (yyyymmdd).
Minor consideration is if the time string should be limited to year 0000 to 2999 (time if 8 digits number begins with 0, 1, or 2. Otherwise integer).
If 8 digit must be used as the value of a variable, an extra 0 should be added in order to inform MET the given string to be an integer, not time string:

@20230101: unix string  2023-01-02
@020230101: integer 20230101, not yyyymmdd

@hsoh-u hsoh-u added reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project and removed alert: NEED ACCOUNT KEY Need to assign an account key to this issue labels Mar 3, 2023
@hsoh-u hsoh-u moved this from 🔖 Ready to 🏗 In progress in MET-11.1.0 Development Mar 3, 2023
hsoh-u pushed a commit that referenced this issue Mar 6, 2023
hsoh-u pushed a commit that referenced this issue Mar 6, 2023
@hsoh-u hsoh-u moved this from 🏗 In progress to 👀 In review in MET-11.1.0 Development Mar 9, 2023
@JohnHalleyGotway JohnHalleyGotway moved this from 🔖 Ready to 👀 In review in Coordinated METplus-5.0 Support Mar 9, 2023
hsoh-u added a commit that referenced this issue Mar 9, 2023
hsoh-u added a commit that referenced this issue Mar 9, 2023
…dd_main_v11.0

Bugfix #2482 main_v11.0 time slicing yyyymmdd
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in MET-11.1.0 Development Mar 9, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Coordinated METplus-5.0 Support Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MET: Library Code priority: medium Medium Priority reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project requestor: Community General Community type: bug Fix something that is not working
Projects
Status: Done
3 participants