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

ExchangeCalendars and TradingSchedules #1138

Merged
merged 38 commits into from Jun 9, 2016

Conversation

Projects
None yet
8 participants
@jfkirk
Contributor

jfkirk commented Apr 18, 2016

This PR adds two new base classes: ExchangeCalendar and TradingSchedule.

An ExchangeCalendar is an object that manages timing information for a given asset exchange. It can be queried to find out if an exchange is currently trading, or to get more general information about the trading on the exchange.

A TradingSchedule is an object that manages timing information for a TradingAlgorithm. It tells the algorithm, via the clock, when to execute.

This PR contains ExchangeCalendars for some exchanges, including NYSE, and an implementation of TradingSchedule that wraps an ExchangeCalendar for backwards compatibility.

@jfkirk

This comment has been minimized.

Contributor

jfkirk commented Apr 18, 2016

Next step is to delete the old tradingcalendar module.

# It's not a lazy calendar, so raise an exception
raise InvalidCalendarName(calendar_name=name)
if name is 'NYSE':

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Please don't use is for string comparisons. This is effectively a random number generator.

"""
A TradingSchedule defines the execution timing of a TradingAlgorithm.
"""
__metaclass__ = ABCMeta

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

This doesn't do anything in Python3. Use six.with_metaclass

def __init__(self):
# Assign the partial calendar helpers
self.next_execution_day = partial(

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Why are these all implemented as partials? Seems like they could just be methods that access members of self?

This comment has been minimized.

@jfkirk

jfkirk Apr 18, 2016

Contributor

The methods are shared between the ExchangeCalendar and the TradingSchedule, and the names of the member methods are different between those objects.

This is one of the reasons that we may remove the TradingSchedule before merging. We will be discussing that in the next few days.

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

What does that have to do with implementing these as partials? I'm asking why the implementation of this isn't something like:

def next_execution_day(self, date):
    return next_scheduled_day(date, last_trading_day=self.last_execution_day, is_scheduled_day_hook=self.is_executing_on_day)

This comment has been minimized.

@jfkirk

jfkirk Apr 18, 2016

Contributor

Oh, sorry - misunderstood the statement. Do you believe that implementing this as in your comment would be clearer? If so, I will modify them accordingly.

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

They way these are written, we can't have docstrings for the methods, and you can't introspect them from the class, because they're not defined until you construct an instance.

Some (most?) of the implementations here also feel not worth delegating at all. {next,previous}_open_and_close are literally just calling the two functions you're passing in. Breaking that out into another method is just adding overhead.

next,previous_scheduled_day are much less efficient than they should be because of this interface as well. By only exposing an "are we executing today?" predicate, you're forcing the lookup algorithm to be a linear search.

This comment has been minimized.

@jfkirk

jfkirk Apr 18, 2016

Contributor

Makes sense - thank you. I will re-write accordingly.

raise NotImplementedError()
@property
@remember_last

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

This is just lazyval.

This comment has been minimized.

@llllllllll

llllllllll Apr 18, 2016

Member

the cache will be invalidated if a subclass accesses it, so it is slightly different, though I think that is not intended.

Returns
-------
Timestamp or None
The data availability time on the given date, or None if there is

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

When would this return None?

raise NotImplementedError()
@abstractmethod
def is_executing_on_minute(self, dt):

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Seems like this should be called should_execute_on_minute, given that that's the phrasing used everywhere in the docs and description.

raise NotImplementedError()
@abstractmethod
def is_executing_on_day(self, dt):

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

ditto above.

def day(self):
"""
A CustomBusinessDay defining those days on which the algorithm is
usually trading.

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

what does "usually" mean in this context?

raise NotImplementedError()
@abstractmethod
def minute_window(self, start, count, step=1):

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Do we not have enough information to implement this in terms of the abstract intefaces of the class? (It seems like we should be able to do so)

raise NotImplementedError()
class ExchangeTradingSchedule(TradingSchedule):

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Why does this class exist if it's just a proxy around an ExchangeCalendar?

return self._exchange_calendar.early_closes
class NYSETradingSchedule(ExchangeTradingSchedule):

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Ditto above.

days_of_week=(MONDAY, TUESDAY, THURSDAY),
start_date=Timestamp("1995-01-01"),
)
FridayAfterIndependenceDayExcept2013 = Holiday(

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Most of these oddities were developed by hand by checking against the history of the NYSE. Does the CME follow exactly the NYSE schedule?

September11Closings = date_range('2001-09-11', '2001-09-16', tz='UTC')
# http://en.wikipedia.org/wiki/Hurricane_sandy
HurricaneSandyClosings = date_range(

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Was the CME closeed for sandy?

This comment has been minimized.

@StewartDouglas

StewartDouglas Apr 18, 2016

Contributor

I previously thought it was, but more digging revealed that trading was just 'disrupted', so the code as it stands is out of date. See this for more info: https://github.com/quantopian/dataloader/blob/futures-exp-dates/dataloader/resources/tradingcalendar_cme.py#L254-L274

@@ -54,6 +58,9 @@
MAX_WEEK_RANGE = 5
_static_nyse_cal = get_calendar('NYSE')

This comment has been minimized.

@llllllllll

llllllllll Apr 18, 2016

Member

does this mean that none of the schedule function rules work if you use a different calendar?

if 'trading_schedule' in config:
trading_schedule = config['trading_schedule']
else:
trading_schedule = default_nyse_schedule

This comment has been minimized.

@llllllllll

llllllllll Apr 18, 2016

Member

config.get('trading_schedule', default_nyse_schedule)

return distance
def minutes_for_day(day, open_and_close_hook):

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

I don't think this is adding any meaningful value as a standalone function.

return pd.date_range(start, end, freq='T')
def days_in_range(start, end, all_days):

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

Was this the previously-existing implementation? This is doing four linear scans of a large array, when it should be doing two binary searches. The right implementation of this is:

return all_days[all_days.slice_indexer(start_date, end_date)]
bool
True if exchange is open at any time during the day containing @dt
"""
dt_normalized = normalize_date(dt)

This comment has been minimized.

@ssanderson

ssanderson Apr 18, 2016

Member

When do we use session_date and when do we use normalize_date?

@jfkirk jfkirk force-pushed the exchange-calendars-v2 branch 2 times, most recently from 147fd10 to b49c022 May 6, 2016

@coveralls

This comment has been minimized.

coveralls commented May 9, 2016

Coverage Status

Coverage decreased (-1.7%) to 79.789% when pulling 7b5a187 on exchange-calendars-v2 into 0562179 on master.

@jfkirk jfkirk force-pushed the exchange-calendars-v2 branch from cc0d906 to 397d973 May 10, 2016

@coveralls

This comment has been minimized.

coveralls commented May 10, 2016

Coverage Status

Coverage decreased (-1.7%) to 79.875% when pulling 397d973 on exchange-calendars-v2 into 0ecf3a0 on master.

@@ -603,7 +620,7 @@ def run(self, data=None, overwrite_sim_params=True):
self.data_portal = DataPortal(
self.trading_environment,

This comment has been minimized.

@jfkirk

jfkirk May 11, 2016

Contributor

This should be a trading_schedule.

@coveralls

This comment has been minimized.

coveralls commented May 11, 2016

Coverage Status

Coverage decreased (-1.6%) to 79.917% when pulling a7da883 on exchange-calendars-v2 into 0ecf3a0 on master.

@coveralls

This comment has been minimized.

coveralls commented May 11, 2016

Coverage Status

Coverage decreased (-1.6%) to 79.917% when pulling 0a7da8b on exchange-calendars-v2 into 0ecf3a0 on master.

@coveralls

This comment has been minimized.

coveralls commented May 11, 2016

Coverage Status

Coverage decreased (-1.6%) to 79.917% when pulling 0a7da8b on exchange-calendars-v2 into 0ecf3a0 on master.

@jfkirk jfkirk force-pushed the exchange-calendars-v2 branch from 0a7da8b to a7da883 May 11, 2016

jfkirk and others added some commits May 25, 2016

Fixes for CMEExchangeCalendar
Note that a lot of this duplicates what we have for
NYSEExchangeCalendar.
DEV: Cleaned up `trading_minute_window`
Removed it from ExchangeCalendar.

Fixed TradingSchedule’s implementation to be much faster.  Removed the
`step` parameter.
MAINT: Removes the set_first_trading_day method of DataPortal
Since the first trading day is now passed directly to the DataPortal on
init, there's no need for a method that does this. Moves all the
additional logic/assignments into the init. Also corrects an issue where
we would never create certain attributes if self._first_trading_day was
None.

Adds the ability to specify the first trading day for a data portal in a
test case when using the WithDataPortal fixture.

@jbredeche jbredeche force-pushed the exchange-calendars-v2 branch from e536936 to 02a91ec Jun 8, 2016

@coveralls

This comment has been minimized.

coveralls commented Jun 8, 2016

Coverage Status

Coverage decreased (-1.7%) to 81.05% when pulling b5633aa on exchange-calendars-v2 into c9b5979 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 8, 2016

Coverage Status

Coverage decreased (-1.7%) to 81.05% when pulling 60cd4aa on exchange-calendars-v2 into c9b5979 on master.

jbredeche added some commits Jun 9, 2016

Merge pull request #1270 from quantopian/just-kidding-put-old-calenda…
…r-back

REV: Restore old tradingcalendar.py
@coveralls

This comment has been minimized.

coveralls commented Jun 9, 2016

Coverage Status

Coverage decreased (-1.7%) to 81.05% when pulling 4b09715 on exchange-calendars-v2 into c9b5979 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 9, 2016

Coverage Status

Coverage decreased (-2.4%) to 80.436% when pulling 7da1eae on exchange-calendars-v2 into c9b5979 on master.

@jbredeche jbredeche merged commit 5a6b870 into master Jun 9, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jbredeche jbredeche deleted the exchange-calendars-v2 branch Jun 9, 2016

@jfkirk

This comment has been minimized.

Contributor

jfkirk commented Jun 9, 2016

Nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment