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

DELETE ME #2433

Closed
wants to merge 2 commits into from
Closed

DELETE ME #2433

wants to merge 2 commits into from

Conversation

JohnHalleyGotway
Copy link
Collaborator

Same fix as PR #2432 but for the develop branch instead. Please review them together.

@JohnHalleyGotway JohnHalleyGotway added this to the MET 11.1.0 milestone Feb 2, 2023
@JohnHalleyGotway JohnHalleyGotway linked an issue Feb 2, 2023 that may be closed by this pull request
23 tasks
@JohnHalleyGotway JohnHalleyGotway changed the title Bugfix #2426 main_v11.0 buoy Bugfix #2426 develop buoy Feb 2, 2023
@davidalbo
Copy link
Contributor

@JohnHalleyGotway does the most recent comment from GwenChen in the issue here #2426
change anything as regards this pull request?

@JohnHalleyGotway
Copy link
Collaborator Author

JohnHalleyGotway commented Feb 3, 2023

@davidalbo, I suppose it does. I'd recommend that we stick with the smaller set of changes in #2432 for the bugfix in main_v11.0. But ideally we'd update the develop branch with the more thorough set of the buoy locations.

Options are...

  1. Pull back this PR and work on the more complete fix for develop now.
  2. Approve/merge this PR now, but write up a separate issue to update the buoy locations file sometime later.

I don't have time now to work on the updated buoy locations file, but perhaps you do? If you do, let's add you as an assignee to this issue and do option (1). If not, let's do option (2).

What do you think?

@JohnHalleyGotway JohnHalleyGotway marked this pull request as draft February 3, 2023 22:36
@JohnHalleyGotway
Copy link
Collaborator Author

Here's the plan:

  1. @davidalbo proceeds with the review of PR Bugfix #2426 main_v11.0 buoy #2432 to fix this in the main_v11.0 branch.
  2. @JohnHalleyGotway converts this PR DELETE ME #2433 to a draft.
  3. @davidalbo checks out the bugfix_2426_develop_buoy branch and continues development on it. The goal is to modify the contents of ndbc_stations.xml using the data from ndbc.noaa.gov/to_station.shtml, as described in this issue comment.
    Please check to see if any of the lat/lon/elevation information changes from the original and this new source.
  4. Once the changes are ready @davidalbo click the "Ready for review" button on this PR.

@JohnHalleyGotway
Copy link
Collaborator Author

@davidalbo wondering if I should just close this PR? Or are you planning on using it for your set of changes for develop for this issue?

@davidalbo
Copy link
Contributor

@JohnHalleyGotway I have python already implemented and uploaded to this branch:  bugfix_2426_develop_buoy. All that is needed is testing, which would be to run the python script, then run ascii2nc on the newer ndbc locations file that gets generated.And... replace the actual table file with the newer one if the test works.

I can do this as part of issue 2426 I guess, whatever makes sense. I'm actually confused by what seems to be 3 issues for this. 2426 2433 2432

Copy link
Contributor

@davidalbo davidalbo left a comment

Choose a reason for hiding this comment

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

If I'm understanding, this pull request puts a few things into develop:

  1. adds changes made by JohnHG for logging formatting.
  2. Adds additional stations by hand to the ndbc_stations.xml file.
  3. Adds a python script that combines station information from the two sources, but that is not yet used for anything.

We can continue working on #2426 to actually use the python or make a new issue to continue work on that.

I approve this pull request.

@JohnHalleyGotway JohnHalleyGotway requested review from davidalbo and removed request for davidalbo February 23, 2023 18:00
@JohnHalleyGotway JohnHalleyGotway changed the title Bugfix #2426 develop buoy DELETE ME Feb 23, 2023
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.

2 participants