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

ENH: time/calendar business logic improvements #1346

Merged
merged 10 commits into from Aug 4, 2016

Conversation

Projects
None yet
4 participants
@jbredeche
Member

jbredeche commented Jul 24, 2016

I pulled in 03f120d because I needed it on this branch, so once that PR is in, I can rebase that commit out.

The goal of this PR was to augment data.can_trade to also check whether the asset's exchange is open. To do that, I also needed to move before_trading_start to fire at 8:45am (ie, possibly an exchange minute) instead of midnight UTC (a session label), so that got put into this PR too.

@jbredeche jbredeche force-pushed the check-exchange-time branch from ef5200d to 30f4f18 Jul 25, 2016

@coveralls

This comment has been minimized.

coveralls commented Jul 25, 2016

Coverage Status

Coverage decreased (-0.06%) to 83.493% when pulling 144ca3a on check-exchange-time into a534255 on master.

@jbredeche jbredeche force-pushed the check-exchange-time branch from eef4bcc to 4db6c00 Jul 25, 2016

@coveralls

This comment has been minimized.

coveralls commented Jul 25, 2016

Coverage Status

Coverage decreased (-0.05%) to 83.507% when pulling 4db6c00 on check-exchange-time into 2fe94d0 on master.

@jbredeche jbredeche force-pushed the check-exchange-time branch from 4db6c00 to ab75eed Jul 25, 2016

@@ -512,7 +513,7 @@ def initialize(context):
attach_pipeline(pipeline, 'test')
def handle_data(context, data):
today = get_datetime()
today = normalize_date(get_datetime())

This comment has been minimized.

@ehebert

ehebert Jul 25, 2016

Member

Does this mean that existing pipeline algos that do not normalize the date, will have changing results?

@jbredeche jbredeche force-pushed the check-exchange-time branch from ab75eed to 9ca2727 Jul 26, 2016

@coveralls

This comment has been minimized.

coveralls commented Jul 26, 2016

Coverage Status

Coverage increased (+0.03%) to 84.859% when pulling 9ca2727 on check-exchange-time into 52f3e91 on master.

dt_value = normalize_date(dt).value
else:
dt_value = dt.value
ref_start = self.start_date.value

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

What calendar's sessions do these boundary values correspond to? Will futures contracts have start/end dates (session labels) that mean they expire at the close of the exchange they trade on?

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

Yes. asset lifetimes are defined as a start and end session. and futures contracts will expire at the end of a session.

calendar = self._exchange_trading_calendar_for_asset()
return calendar.is_open_on_minute(dt_minute)
def _exchange_trading_calendar_for_asset(self):

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Naming-wise, Asset._exchange_trading_calendar_for_asset strikes me as redundant.

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

agreed, updated.

"COMEX": "CME",
"NYMEX": "CME",
"ICEUS": "ICE",
"NYFE": "ICE"

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Do you think this mapping belongs in zipline, or should the creator of the data have mapped it already?

import pandas as pd
class DailyHistoryAggregator(object):

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Are there changes in here?

This comment has been minimized.

@jbredeche

jbredeche Jul 26, 2016

Member

no, just moved the class into its own file.

This comment has been minimized.

@ehebert

ehebert Jul 27, 2016

Member

Can we do this in a separate PR or commit?

This comment has been minimized.

@ehebert

ehebert Jul 27, 2016

Member

There is a change, normalize_date was changed to using session_label, working on a quick PR now to break out the move to a sep module so that difference is shown.

This comment has been minimized.

@ehebert

ehebert Jul 27, 2016

Member
regular_minutes = self.minutes_by_session[session]
# we have to search anew every session, because there is no
# guarantee that any two session start on the same minute

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Could you clarify this?

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

Sure. A trading calendar can have late opens, which means that for two trading sessions, the index of the bts_minute in the sessions' minutes can be different.

This comment has been minimized.

@richafrank

richafrank Jul 27, 2016

Member

Ah ok cool. Could you include that in the comment? - just wasn't clear to me. (Also typo "two session start")

for minute in regular_minutes[bts_idx:]:
yield minute, BAR
if minute_emission:
yield minute, MINUTE_END

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Instead of repeating this 3 times over different iterables, would it make sense to factor out a function?

This comment has been minimized.

@jbredeche
handle_benchmark(normalize_date(dt))
execute_order_cancellation_policy()
yield self._get_daily_message(dt, algo, algo.perf_tracker)
elif action == BEFORE_TRADING_START_BAR:
# call before trading start
algo.on_dt_changed(dt)

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

If the sim engine might emit bars within a session both before and after BTS, might the dt not change here?

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

yes. this just ensures that the algo dt is right, even if it's the same dt.

this ensures that the portfolio and account info are up to date if they are being accessed in before_trading_start.

if bts_idx == len(regular_minutes):
# before_trading_start is after the last close, so don't emit
# it

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

If a session runs from BTS time to BTS time the next day, would we not run BTS?

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

I made this more explicit, by checking if bts_minute > regular_minutes[-1]

"""
if isinstance(self, Equity):
# FIXME: probably too Quantopian-specific
return get_calendar("NYSE")

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Are we planning to move the calendars into the asset db + materialized instances? My expectation was that we would look up the exchange on the asset.

This comment has been minimized.

@jbredeche

jbredeche Aug 2, 2016

Member

yup. The asset db now has the canonical exchange name ("NASDAQ", "CME", etc) and we use that to look up the calendar.

@@ -497,28 +497,33 @@ def _create_clock(self):
trading_o_and_c = self.trading_calendar.schedule.ix[
self.sim_params.sessions]
market_closes = trading_o_and_c['market_close'].values.astype(np.int64)
minutely_emission = False

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Looks like this value is only used in the else. Could move it there for parallel constructions between the if and the else, but not a big deal.

)
)
session_label = normalize_date(dt) # FIXME
if not asset._is_alive_for_session(session_label):

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Naming-wise, looks like these underscore methods of Asset are actually public, in that consumers outside Asset call them?

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

good call, they are public, fixed the naming.

@@ -187,37 +201,58 @@ cdef class Asset:
"""
return cls(**dict_)
def _is_alive(self, dt, bool normalized):
def _is_alive_for_session(self, session_label):

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Looks like each call site for this function calls session_for_minute immediately before. Should we move that into here?

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

I prefer keeping this method simple - it's responsible for taking a session and returning whether the asset was alive for that session.

@@ -89,6 +101,8 @@ cdef class Asset:
self.first_traded = first_traded
self.auto_close_date = auto_close_date

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

I guess we don't have flake8 for cython, but probably don't want these extra lines.

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

indeed.

class WarningsCatcher(catch_warnings):
"""
Subclass of warnings.catch_warnings that takes a list of warning types to
ignore.

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Should we use warnings's filters to manage this instead?

log = []
def showwarning(*args, **kwargs):
if args[1] in self._types_to_ignore:

This comment has been minimized.

@richafrank

richafrank Jul 26, 2016

Member

Looks like this is the only intended difference from the base class, but there are other differences from the python3 version. Since the intended difference is governed by self._record, which is always True for the subclass, I recommend actually setting it to False, and using:

def __enter__(self):
    super(WarningsCatcher, self).__enter__()

    log = []
    def showwarning(*args, **kwargs):
        if args[1] in self._types_to_ignore:
            return
        log.append(WarningMessage(*args, **kwargs))

    self._module.showwarning = showwarning
    return log

which isolates the differences from the base class.

This comment has been minimized.

@jbredeche

jbredeche Jul 27, 2016

Member

good call, done

@coveralls

This comment has been minimized.

coveralls commented Jul 28, 2016

Coverage Status

Coverage increased (+0.03%) to 84.859% when pulling 27288ae on check-exchange-time into 52f3e91 on master.

@jbredeche jbredeche force-pushed the check-exchange-time branch 2 times, most recently from 0ab8049 to fdef50c Jul 29, 2016

@coveralls

This comment has been minimized.

coveralls commented Jul 29, 2016

Coverage Status

Coverage increased (+0.02%) to 85.119% when pulling fdef50c on check-exchange-time into a937d6e on master.

@jbredeche jbredeche force-pushed the check-exchange-time branch from fdef50c to 413c3a0 Aug 2, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.02%) to 85.54% when pulling 413c3a0 on check-exchange-time into ecac6e9 on master.

@jbredeche jbredeche force-pushed the check-exchange-time branch from 4e0364f to 7d4b19a Aug 3, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-0.03%) to 85.46% when pulling 90d1185 on check-exchange-time into 164bd06 on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.03%) to 85.46% when pulling 36e167d on check-exchange-time into 164bd06 on master.

@jbredeche jbredeche force-pushed the check-exchange-time branch from 36e167d to d1077a3 Aug 4, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.03%) to 85.46% when pulling d1077a3 on check-exchange-time into 164bd06 on master.

@jbredeche jbredeche merged commit 8a44231 into master Aug 4, 2016

2 checks passed

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

@richafrank richafrank deleted the check-exchange-time branch Aug 22, 2016

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