-
Notifications
You must be signed in to change notification settings - Fork 135
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
Sessions to tz-naive? Schedule times to UTC? #42
Comments
I included a fix to the merged #41 that addressed this FutureWarning. I believe it closes this issue. |
I think what you did closes the immediate problem, but I'd like to think more about defining the target dtype to be Morally everything here should be in UTC I think? Especially internally. |
I did wonder if you opened this for the UTC question. I've been half expecting to come across something that would give me a mini-eureka moment of understanding as to why sessions are UTC and opens, closes etc are tz-naive? Haven't had it yet.
I suspect that changing opens, closes etc. to UTC and/or sessions to tz-naive would involve far more than changing a few lines of code. Also, I imagine it would break a lot of All that said, if you decide you would like to make sessions tz-naive and/or make open, closes etc. UTC then I'd more than happily have a look at putting together a PR. |
I'd say a lot of this is an artifact of how the original trading calendars library was created https://github.com/quantopian/trading_calendars, its use in zipline, and how far back this was all done (the earliest commits in trading calendars are from 2013, and that was after this was spun out of zipline). So some of this grew out organically, some of this is a lack of/difficulty of tz support back in the mid 2010s. Prior to this fork, we also maintained compatibility with py27/much older versions of pandas and numpy. My personal views are probably that
The main advantage (in my mind) is just consistency. You know whenever you work with the library or are relying on data coming from the library that the timezone is in UTC, you never have to think about it. This definitely is a reflection of my own experience/personal preference, so I'm definitely open to other opinions (though maybe there's no "correct" answer). I agree that this would be a bigger change, both in work and interface, and is more suitable to a longer term 4.0 release and not something we should do lightly. |
Big +1 on this. I'm working on a library that creates price data sets that can include instruments trading in different timezones (will be open source). All the internals are UTC and it would certainly be easier to work with With regards to defining session labels as UTC:
Interesting. My experience led me in the opposite direction, wishing they were tz-naive! For me I think it comes down to the labels not being times, and defining them as UTC confuses this. Maybe this doesn't outweigh advantages of across-the-board consistency. It would be interesting if other users were to offer their thoughts on this one. I'm due to have a good tidy up of the library I'm working on - I'll add to this conversation anything I come across that informed my view. In the meantime, would it be reasonable to start a Release 4.0 issue with a todo along the lines of... Change opens, closes, break_starts and break_ends to UTC. ? |
Summary of notes above in favour of tz-naive sessions:
Work on the 'side' option and new TestBase has again had me thinking that sessions should be tz-naive... Internals Moving forwards I'd anticipate the internals looking at session nanos rather than the timestamps (discussion #87). This would make the question more a matter of how sessions should be represented externally. Externals As for output, I can't see any case where a session that's spat out would be compared with a time. Indeed, if a user were comparing a session with a time then it's likely they haven't understood the concept of a session. In my experience, session output is more likely to be used to compare against tz-naive dates. In this case users have to convert the session output back to tz-naive (otherwise you get the 'cannot compare a tz-naive timestamp with a tz-aware timestamp' error). In short, IMHO defining sessions as UTC is forcing consistency on concepts that aren't consistent. Consequently:
|
This test might be a good example of how having tz-aware sessions can give rise to confusion. The test mistakenly assigns a local timezone to every expected holiday and then tries to verify that those expected holidays are not sessions by looking for them in |
Yep! I'm convinced of your point actually, this is definitely confusing what a session is. |
Another issue following this latest pandas release is that the ExchangeCalendar constructor is now raising a FutureWarning.
The cause is within the creation of the schedule
DataFrame
, due to conversion of DatetimeIndex values with dtype'datetime64[ns, UTC]'
(self._opens etc.) to'datetime64[ns]'
.One option would be to change the target dtype to "datetime64[ns, UTC]" thereby defining the schedule columns in terms of UTC. This additional context would be meaningful although I suspect it would open a can of worms with respect to changes it would necessitate within
exchange_calendars
and its clients. For now I'll add a fix to PR #41 that retains the existing behaviour.Originally posted by @maread99 in #40 (comment)
The text was updated successfully, but these errors were encountered: