-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add NDBC simple web service #241
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall there's a lot of stuff here, but such is life when processing ascii data. Could probably refactor around common operations to all the parsers, but I don't think it's worth the effort.
siphon/simplewebservice/ndbc.py
Outdated
super(NDBC, self).__init__('https://www.ndbc.noaa.gov/') | ||
|
||
@classmethod | ||
def station_observations(cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, this method is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over from spelling internal debating 😄
siphon/simplewebservice/ndbc.py
Outdated
return available_data | ||
|
||
@classmethod | ||
def raw_buoy_data(cls, buoy, type='txt'): # noqa: A002 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about going with data_type
instead of type
so we can avoid overriding a built-in (and the flake8-plugin checking for it).
I've addressed the changes, switched to using pandas table parser vs fixed width, added several examples, and tied up a few loose ends here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple comments. We can merge despite that if you really don’t agree.
examples/ndbc/buoy_met_request.py
Outdated
|
||
#################################################### | ||
# Let's make a simple time series plot to checkout what the data look like. | ||
fig = plt.figure(figsize=(12, 10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer this:
fig, ax = plt.subplots(3, 1, figsize=(12, 10))
ax2b = ax[2].twinx()
ax[1].plot(...)
...
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that personally - it's CS pure, but not how a scientist would implement a plot. I often named my axes how they would be labeled in the figure (a, b, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be convinced, but it's minor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to label them that way, great. I have a pretty intense aversion to using numbers in variable names where an array will do—especially when you can get an array out of an API call. This is literally an example from my Pythonic plotting workshop unit.
You can still accomplish what you want with:
fig, (a, b, c) = plt.subplots(3, 1, figsize=(12, 10))
How’s that read to you?
I’m not interested in how a “typical” scientist would do this—I’m interested in implementing best practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we'll need to change how it's done elsewhere in our workshop materials :)
siphon/simplewebservice/ndbc.py
Outdated
'3hr_pressure_tendency': 'hPa', | ||
'time': None} | ||
|
||
df = pd.read_fwf(StringIO(content), skiprows=2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional that this is still using fwf
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope
What did the table parser get you? |
Table parser avoided the weirdness of the wind speed cutting off the tens digit unless there is 10 m/s or greater in the first 100 rows.... because the file is justified strangely. |
This simple web service provides access to the latest observations file for all buoys, as well as the realtime buoy data for 9 different data types.