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

Feature 2511 ndbc locations update #2605

Merged
merged 3 commits into from
Jul 10, 2023

Conversation

davidalbo
Copy link
Contributor

Expected Differences

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

    If yes, please describe: The python script build_ndbc_stations.py has a new command line option -e <filename> which allows specification of the current stations file to compare against content pulled from the web. The default value can be seen by entering the command option -h. The default value assumes you are running from the script directory, and using the current stations data/table_files/ndbc_stations.xml in the MET repo that is checked out.

  • 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: The script was run, it pulls content from the web. The pulled web content can now be parsed without error (the web page format changed). Comparison of the newly created station file against the existing one (data/table_files/ndbc_stations..xml) showed changes. Running this on successive days showed some changes every time it was run. The buoys are drifting. I created the latest stations file and replaced the ndbc_stations.xml file with the locations as of today 2023/07/10

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions: Recommend you run the script with no command line arguments from the directory in which the script is found (cd scripts/python/utility; build_ndbc_stations.py). It should pull content from the web and create a new stations file merged.txt. If nothing has changed it will be identical to data/table_files/ndbc_stations.xml, otherwise you'll see some drifting locations. Also, you can try setting some of the command options for additional tests, i.e. play around with that

  • 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? [Maybe]

    If yes, describe the new output and/or changes to the existing output: ascii2nc has a unit test that reads NDBC stations, which will read the changed ndbc_stations.xml file from share/met/table_files. If the particular NDBC stations in the test happen to have drifted, it could change the output. Only a few stations seem to be drifting.

  • Please complete this pull request review by [July 10].

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 Development issue 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.

… option to specify the existing stations file to use, general cleanup
…' for the script build_ndbc_stations_from_web.py
…hanging on a daily basis (drifting bouys most likely)
@davidalbo davidalbo added type: bug Fix something that is not working type: enhancement Improve something that it is currently doing alert: NEED ACCOUNT KEY Need to assign an account key to this issue MET: Configuration priority: high High Priority labels Jul 10, 2023
@davidalbo davidalbo added this to the MET 11.1.0 milestone Jul 10, 2023
@davidalbo davidalbo self-assigned this Jul 10, 2023
@davidalbo davidalbo linked an issue Jul 10, 2023 that may be closed by this pull request
20 tasks
Copy link
Collaborator

@georgemccabe georgemccabe left a comment

Choose a reason for hiding this comment

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

These changes seem good to me. A few minor suggestions: I noticed a few commented lines of code that could be removed. I also noticed that the setLat and setLon functions are very similar and could be refactored to 1 function that is called for each lat and lon.

Question: Is the date/time when the script is run saved anywhere in the output file? It seems like this would be important to retrospective runs. For example, if re-running a case from a year ago, an updated buoy station file would no longer be accurate since the file does not reflect the buoy locations that corresponds to the data being processed.

@davidalbo
Copy link
Contributor Author

Thanks @georgemccabe these are good suggestions. As for the date being put into the file, that could be done easily as it's XML, so would still be parsable. Right now we maintain a single stations file that changes through time. We could maintain multiple such files, each a 'snapshot' of some moment in time. The ascii2nc code could be changed to find the stations file closest in time to the data being studied. But...We can not go back and do this now for previous times as the web content is always real time only. We cannot find out where the stations used to be. If we did something like this, choices as to how often to add a new file, how many to store, etc. would need to be made. Still not perfect. It's a problematic data design for the scientists, in my opinion.

@davidalbo davidalbo merged commit 9614f28 into main_v11.1 Jul 10, 2023
@davidalbo davidalbo deleted the feature_2511_ndbc_locations_update branch July 10, 2023 19:51
JohnHalleyGotway pushed a commit that referenced this pull request Jul 10, 2023
Co-authored-by: John Halley Gotway <[email protected]>
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: jprestop <[email protected]>
Co-authored-by: Daniel Adriaansen <[email protected]>
Co-authored-by: John and Cindy <[email protected]>
Co-authored-by: Howard Soh <[email protected]>
Co-authored-by: George McCabe <[email protected]>
Co-authored-by: hsoh-u <[email protected]>
Co-authored-by: MET Tools Test Account <[email protected]>
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: lisagoodrich <[email protected]>
Co-authored-by: davidalbo <[email protected]>
Co-authored-by: Lisa Goodrich <[email protected]>
Co-authored-by: metplus-bot <[email protected]>
Co-authored-by: j-opatz <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jonathan Vigh <[email protected]>
Co-authored-by: Tracy Hertneky <[email protected]>
Fix Python environment issue (#2407)
fix definitions of G172 and G220 based on comments in NOAA-EMC/NCEPLIBS-w3emc#157. (#2406)
fix #2380 develop override (#2382)
fix #2408 develop empty config (#2410)
fix #2390 develop compile zlib (#2404)
fix #2412 develop climo (#2422)
fix #2437 develop convert (#2439)
fix for develop, for #2437, forgot one reference to the search_parent for a dictionary lookup.
fix #2452 develop airnow (#2454)
fix #2449 develop pdf (#2464)
fix #2402 develop sonarqube (#2468)
fix #2426 develop buoy (#2475)
fix 2518 dtypes appf docs (#2519)
fix 2531 compilation errors (#2533)
fix #2531 compilation_errors_configure (#2535)
@JohnHalleyGotway JohnHalleyGotway removed type: bug Fix something that is not working type: enhancement Improve something that it is currently doing alert: NEED ACCOUNT KEY Need to assign an account key to this issue MET: Configuration priority: high High Priority labels Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update NDBC locations prior to the MET-11.1.0 release
3 participants