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.

Sign up for GitHub

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

Jump to bottom

tutorial workflow - added alternative wind obs data retrieval #265

Merged
merged 12 commits into from
Sep 8, 2021
Merged

tutorial workflow - added alternative wind obs data retrieval #265

merged 12 commits into from
Sep 8, 2021

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jul 14, 2021 •
edited
Loading

Fixes part of the dependency on MetOffice datapoint.

  • Get the wind CSV from recent observations stored in a fairly reliable (ogimet) third party source, leaving existing setup as a fallback.
  • Get the observations lat-long data from a file in the repo.
  • Swapped Belmullet for Aldergrove, because it's more available.
  • Provided a mechanism to get an alternative obs site if the first one doesn't have wind data available.

@wxtim wxtim self-assigned this Jul 14, 2021
@wxtim wxtim added the bug Something isn't working label Jul 14, 2021
@wxtim wxtim requested a review from MetRonnie July 23, 2021 08:13
MetRonnie
MetRonnie reviewed Jul 23, 2021
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

The tutorial content needs updating (still has Belmullet) - https://cylc.github.io/cylc-doc/nightly/html/tutorial/scheduling/datetime-cycling.html#gathering-observations

Not sure what is happening when I'm running the cylc/doc/etc/tutorial/cylc-forecasting-workflow workflow. I'm getting a messed up wind plot image - https://github.com/cylc/cylc-doc/issues/275 (I posted this as a separate issue not realising I was still checked out on this branch. On master it's not producing the plot of course)

@MetRonnie MetRonnie mentioned this pull request Jul 23, 2021
Closed
@wxtim wxtim requested review from kinow and MetRonnie and removed request for oliver-sanders September 6, 2021 07:52
kinow
kinow approved these changes Sep 6, 2021
View reviewed changes
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Don't know how to run the code, but reading the code it looks OK. Found one thing that probably needs fixing, but everything else is 🆗 🙆‍♂️

Thanks!

cylc/doc/etc/tutorial/cylc-forecasting-workflow/bin/get-observations Outdated Show resolved Hide resolved
wxtim and others added 3 commits September 7, 2021 07:31
@wxtim @kinow
Update cylc/doc/etc/tutorial/cylc-forecasting-workflow/bin/get-observ…
bae0133 
…ations

Co-authored-by: Bruno P. Kinoshita <[email protected]>
@wxtim
Refs to Belmullet ⇒ Aldergrove; WMO number 3976 ⇒ 3917
a0c896a
@wxtim
Merge branch 'tutorial.get_wind_obs_from_hem_hem' of github.com:wxtim…
c07e2c2 
…/cylc-doc into tutorial.get_wind_obs_from_hem_hem

* 'tutorial.get_wind_obs_from_hem_hem' of github.com:wxtim/cylc-doc:
  Update cylc/doc/etc/tutorial/cylc-forecasting-workflow/bin/get-observations
@wxtim
Copy link
Member Author

wxtim commented Sep 7, 2021

The tutorial content needs updating (still has Belmullet) - https://cylc.github.io/cylc-doc/nightly/html/tutorial/scheduling/datetime-cycling.html#gathering-observations

Good spot. I've done some sedding, both to turn "Belmullet" ⇒ "Aldergrove" but to fix the WMO numbers too.

MetRonnie
MetRonnie reviewed Sep 7, 2021
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Still getting messed up wind plots - see the screenshot in #275

cylc/doc/etc/tutorial/cylc-forecasting-workflow/bin/get-observations Outdated Show resolved Hide resolved
@wxtim
Copy link
Member Author

wxtim commented Sep 8, 2021 •
edited
Loading

@MetRonnie

Still getting messed up wind plots - see the screenshot in cylc/cylc-flow#4660

Gotit! (I think). Two missing minus signs in the string.split() indices.

@wxtim wxtim requested a review from MetRonnie September 8, 2021 06:51
MetRonnie
MetRonnie approved these changes Sep 8, 2021
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Those lat/lon were indeed the wrong way round, but I've still got the wind plot bug. Happy to merge this and reopen #275

@MetRonnie MetRonnie merged commit ebb125b into cylc:master Sep 8, 2021
@wxtim wxtim mentioned this pull request Dec 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@kinow kinow kinow approved these changes

Assignees

@wxtim wxtim

Labels
bug Something isn't working tutorials
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wxtim @kinow @MetRonnie