ZeevG / python-forecast.io

A thin Python Wrapper for the Dark Sky (formerly forecast.io) weather API
http://zeevgilovitz.com/python-forecast.io/
Other
423 stars 88 forks source link

Support for naïve datetime objects #30

Closed hack-c closed 9 years ago

hack-c commented 9 years ago

Ref Issue 21.

The .load_forecast() method is currently converting the datetime argument to seconds since the epoch in local time, which has the effect of making it an "aware" datetime. That is, it imbues the datetime with statefulness w.r.t. location that it doesn't necessarily have already, if it's passed as a naïve datetime (i.e. "5 o'clock somewhere", not necessarily "5 o'clock in Greenwich.")

Technically, the issue is that time.mktime() takes an argument in local time (i.e. the locale of my laptop) and converts to seconds since the epoch (docs). In the API though, they expect a naïve datetime, unless timezone is specified. From the API docs:

For the latter format, if no timezone is present, local time (at the provided latitude and longitude) is assumed. (This string format is a subset of ISO 8601 time. An as example, 2013-05-06T12:00:00-0400.)

My thought was, that in order to make the wrapper as thin as possible, that it should behave in the same way. Instead of using mktime(), it should use isoformat().

In [1]: %paste
import time
import datetime
from delorean import Delorean

## -- End pasted text --

In [2]: d = datetime.datetime(2015, 4, 11, 2, 45)  # initialized without any tzinfo

In [3]: d.isoformat()  # this format has no tzinfo, is naïvely unaware of location
Out[3]: '2015-04-11T02:45:00'

In [4]: ep = time.mktime(d.timetuple())

In [5]: ep  # this number is only associated with datetime(2015, 4, 11, 2, 45) in the sense of GMT-04:00 (I'm in New York.) So it's no longer naïve to location.
Out[5]: 1428734700.0

In [6]: dl = Delorean(d, timezone="US/Mountain")  # use delorean to create a timezone-aware datetime.datetime

In [7]: dl.datetime  
Out[7]: datetime.datetime(2015, 4, 11, 2, 45, tzinfo=<DstTzInfo 'US/Mountain' MDT-1 day, 18:00:00 DST>)

In [8]: dl.datetime.isoformat()  # in this case timezone info would be passed on to the API.
Out[8]: '2015-04-11T02:45:00-06:00'
ZeevG commented 9 years ago

This looks really good. Thanks for the pull request.

Quickly running it through the example.py I did run into some problems.

I called load_forecast like this

    time = datetime.datetime.now()
    forecast = forecastio.load_forecast(api_key, lat, lng, time=time)

The url it produced is this https://api.forecast.io/forecast/API_KEY/37.8267,-122.423,2015-04-24T18:18:33.788585?units=auto

And the forecast.io API returned a 400 with the error '{"code":400,"error":"The given location (or time) is invalid."}'

Any ideas what's happening? Interestingly truncating the time to just 2015-04-24T18:18:33 does seem to work.

hack-c commented 9 years ago

Hmm, that's embarrassing, good thing you tried!

I will look at this this morning.

Question, how would you feel about adding some "instrumentation tests" to the test suite that actually call the API? That would cover this case. I guess one would need to provide an API key and that gets complicated...

Thanks Zeev,

Charlie

ZeevG commented 9 years ago

@hack-c any thoughts on this?

I emailed the API developer a few weeks back about getting an additional key for testing but didn't get a response. I could put a key in the repo and keep an eye on it's usage, maybe refresh the key every now and then. Hard to know... I doubt Forecast.io would really appreciate it.

hack-c commented 9 years ago

We should probably avoid pasting in API keys to GitHub... the way it is in example.py looks fine.

Also sorry I never got back to this-- from the above it looks like the fraction of a second is what's tripping up the API, we should probably just truncate the decimal, ya?

hack-c commented 9 years ago

Added a change that strips microseconds using datetime.datetime.replace().

This operation is idempotent, so that if the passed datetime object doesn't have a value microseconds, that's ok.

I edited example.py to take a time argument as you did above, got the following output:

03:32 $ python example.py

13.88
===========Hourly Data=========
Hourly Summary: Partly cloudy starting in the morning, continuing until evening.
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-18 12:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-18 13:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-18 14:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-18 15:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-18 16:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-18 17:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-18 18:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-18 19:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-18 20:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-18 21:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-18 22:00:00>
<ForecastioDataPoint instance: Mostly Cloudy at 2015-05-18 23:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-19 00:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-19 01:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-19 02:00:00>
<ForecastioDataPoint instance: Mostly Cloudy at 2015-05-19 03:00:00>
<ForecastioDataPoint instance: Mostly Cloudy at 2015-05-19 04:00:00>
<ForecastioDataPoint instance: Mostly Cloudy at 2015-05-19 05:00:00>
<ForecastioDataPoint instance: Mostly Cloudy at 2015-05-19 06:00:00>
<ForecastioDataPoint instance: Mostly Cloudy at 2015-05-19 07:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-19 08:00:00>
<ForecastioDataPoint instance: Partly Cloudy at 2015-05-19 09:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-19 10:00:00>
<ForecastioDataPoint instance: Clear at 2015-05-19 11:00:00>
===========Daily Data=========
Daily Summary: None
<ForecastioDataPoint instance: Partly cloudy until evening. at 2015-05-18 12:00:00>
bzillins commented 9 years ago

It seems that datetime.datetime.fromtimestamp() adds in local timezone information as well. I believe self.time, sr_time, and ss_time can return a stamp with effect of the locale of the requesting machine.

http://stackoverflow.com/questions/18812638/get-timezone-used-by-datetime-datetime-fromtimestamp

hack-c commented 9 years ago

@bzillins, using the timezone info of the requesting machine is what we want to avoid I think.

The way the API works, is if no time zone is specified explicitly in the request, e.g. 2015-04-11T02:45:00-06:00 (the -06:00 part), then the API assumes you're giving local time at the coordinates specified. With this pull request I'm arguing the wrapper should work the same way.

The use case where it came up was when I was querying for historical weather data in Colorado while I was sitting in NY; I gave datetimes without tzinfo and was expecting results back in Colorado time, but was getting results in NY time.

Makes sense?

bzillins commented 9 years ago

@hack-c Yes, the blip I am seeing is introducing itself as I am sitting in Orlando querying California weather.

Changing to time.replace(microsecond=0).isoformat() on the request allows for a clean call to forecastio with an accurate UTC time stamp. When the data comes back the .hourly json object is processed by models.py such that forecast.time returns a datetime object instead of the epoch string. It seems that the .fromtimestamp() function uses the local timezone of the computer when converting epoch to datetime object.

The discrepancy between self.time and self .utime was eliminated when I changed models.py to use .utcfromtimestamp()

I changed

try:
    self.time = datetime.datetime.fromtimestamp(int(d['time']))
    self.utime = d['time']
except:
    pass

To:

try:
    self.time = datetime.datetime.utcfromtimestamp(int(d['time']))
    self.utime = d['time']
except:
    pass

I apologize for being so ignorant in my github usage.

hack-c commented 9 years ago

@bzillins excellent, I didn't catch this. Thanks!

Want to push this change?

bzillins commented 9 years ago

It will be my first push, let me figure it out!

ZeevG commented 9 years ago

Cool, thanks for the help!

Sorry I've been so slow to reply. It looks like these two changes should resolve the timezone issues we've had. I'm going to merge this pull request and #32.

hack-c commented 9 years ago

Excellent, thanks @ZeevG !