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

MAINT: Make schedule function rules resilient for no trading day #1226

Merged
merged 1 commit into from Jun 9, 2016

Conversation

Projects
None yet
3 participants
@lianga888
Contributor

lianga888 commented May 23, 2016

Make the rules resilient to env.previous_trading_day or
env.next_trading_day being None

if env.is_trading_day(prev):
return prev.date()
else:
elif env.next_trading_day(prev):
return env.next_trading_day(prev).date()

This comment has been minimized.

@richafrank

richafrank Jun 6, 2016

Member

How fast is next_trading_day? Should we cache the result instead of calling it twice here?

@@ -524,7 +534,7 @@ def get_last_trading_day_of_week(dt, env):
dt = env.next_trading_day(dt)
# Traverse forward until we hit a week border, then jump back to the
# previous trading day.
while dt.date().weekday() > prev.date().weekday():
while dt and dt.date().weekday() > prev.date().weekday():
prev = dt
dt = env.next_trading_day(dt)

This comment has been minimized.

@richafrank

richafrank Jun 6, 2016

Member

Do we need the None check below here too for previous_trading_day?

self.get_last_trading_day_of_month(dt, env),
).date()
self.day = self.get_last_trading_day_of_month(dt, env)
if self.td_delta and self.day:

This comment has been minimized.

@richafrank

richafrank Jun 6, 2016

Member

Now that this method has grown a bit, could we share its common code with get_nth_trading_day_of_month?

This comment has been minimized.

@lianga888

lianga888 Jun 7, 2016

Contributor

Is it worth is to make a base class off which to subclass these two rules (like it is done with the week rule)? Or should it just share the method defined somewhere

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

A base class sounds reasonable to me here.

)
)
except (NoFurtherDataError, TypeError):

This comment has been minimized.

@richafrank

richafrank Jun 6, 2016

Member

When do we expect a TypeError? Since that's such a generic error type, is there any better way to handle it that won't potentially mask something else raising a TypeError?

This comment has been minimized.

@lianga888

lianga888 Jun 7, 2016

Contributor

in events.py, _coerce_datetime raises a TypeError when it tries to coerce a None dt. I could have a raise an new custom error, but is it worth it?

This comment has been minimized.

@richafrank

richafrank Jun 7, 2016

Member

In that case, what do you think about tying the error types to the expected producer, instead of expecting either type from any of the three function calls? This currently says that a TypeError from self.date_func is expected.

)
)
except (NoFurtherDataError, TypeError):
return

This comment has been minimized.

@richafrank

richafrank Jun 6, 2016

Member

Instead of keeping this in sync with below, what do you think about making a starting sentinel value (like None) for next_trading_day? Like elsewhere we have while dt and ...

This comment has been minimized.

@lianga888

lianga888 Jun 7, 2016

Contributor

I'm not sure I understand what you mean by this

self.day = env.add_trading_days(self.td_delta, self.day)
except NoFurtherDataError:
self.day = None
if self.day:

This comment has been minimized.

@richafrank

richafrank Jun 6, 2016

Member

I think we could make this a bit less complex by replacing the conditional with an else:

try:
    self.day = ...
except NoFurtherDataError:
    self.day = None
else:
    self.day = self.day.date()

This comment has been minimized.

@lianga888

lianga888 Jun 7, 2016

Contributor

add_trading_days could potentially return None as well, so we need to account for that

This comment has been minimized.

@richafrank

@lianga888 lianga888 force-pushed the schedule_function_resilience branch 2 times, most recently from 2dd976b to eea1921 Jun 7, 2016

return
if not next_trading_day:
return

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

Now that there are cases when calculate_start_and_end might return without changing self.next_date_end, could should_trigger raise an exception when self.next_date_start and self.next_date_end are None by comparing dt > self.next_date_end?

This comment has been minimized.

@lianga888

lianga888 Jun 8, 2016

Contributor

Yeah, you're right. Will make should_trigger return False if next_date_start is still None after it gets calculated

except NoFurtherDataError:
self.day = None
if self.day:
self.day = self.day.date()

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

Is it intentional that here self.day is assigned a date, but above it's assigned a datetime?

This comment has been minimized.

@lianga888

lianga888 Jun 8, 2016

Contributor

The naming here makes things very confusing. self.date_func actually returns a datetime.date. However env.add_trading_days will return a datetime.datetime, which needs to be coerced. I think everything about this is more complicated than needed. So, I'm going to leave everything as datetime.datetime, which are also all normalized. When we make the comparison in should_trigger, we'll compare to the normalized dt that's passed in

def date_func(self, dt, env):
raise NotImplementedError
def get_trigger_day_of_month(self, dt, env):

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

Glad this is being shared now!

@@ -493,18 +477,14 @@ def get_first_trading_day_of_week(dt, env):
# trading day of the env is also the first trading day of the
# week(in the TradingEnvironment, at least), so just return
# that date.

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

I think this comment is outdated now.

@lianga888 lianga888 force-pushed the schedule_function_resilience branch from eea1921 to 51e72b8 Jun 8, 2016

@coveralls

This comment has been minimized.

coveralls commented Jun 8, 2016

Coverage Status

Coverage increased (+0.01%) to 82.809% when pulling 51e72b8 on schedule_function_resilience into c9b5979 on master.

except NoFurtherDataError:
self.date = None
return self.date

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

If self.month, self.td_delta, and self.date are expected in all subclasses, you might move setting them up to the __init__ of this base class.

while dt.date().weekday() < prev.date().weekday():
# Traverse backward until we hit a week border, then jump back to the
# previous trading day.
while dt and dt.date().weekday() < prev.date().weekday():

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

Not sure how inner this loop is, but I think we can get weekday() from dt itself.

@richafrank

This comment has been minimized.

Member

richafrank commented Jun 8, 2016

Looking good. Just had a couple suggestions.

Also, do we need/have you added tests for the cases you've fixed up during the PR process? Like the date versus datetime issue, or the case where the comparison to None would have raised.

@lianga888 lianga888 force-pushed the schedule_function_resilience branch from 51e72b8 to 96cc00f Jun 8, 2016

@lianga888

This comment has been minimized.

Contributor

lianga888 commented Jun 8, 2016

Yup, added a test for the comparison to None case, and the date versus datetime issue is covered by existing tests. Previously we were comparing two dates; now we're comparing two datetimes

"""
A rule that triggers n days before the last trading day of the month.
"""
def __init__(self, n=0):
if not 0 <= n < MAX_MONTH_RANGE:
raise _out_of_range_error(MAX_MONTH_RANGE)
super(NDaysBeforeLastTradingDayOfMonth, self).__init__(n)
self.td_delta = -n

This comment has been minimized.

@richafrank

richafrank Jun 8, 2016

Member

Probably more consistent with the other attrs to have the base class do the assignment. Could include a flag for whether to invert it. What do you think?

@coveralls

This comment has been minimized.

coveralls commented Jun 8, 2016

Coverage Status

Coverage increased (+0.02%) to 82.815% when pulling 96cc00f on schedule_function_resilience into c9b5979 on master.

Andrew Liang
MAINT: Make schedule function rules resilient for no trading day
Make the rules resilient to env.previous_trading_day or
env.next_trading_day being None

@lianga888 lianga888 force-pushed the schedule_function_resilience branch from 96cc00f to 7b82b9a Jun 9, 2016

@lianga888 lianga888 merged commit 6079604 into master Jun 9, 2016

2 checks passed

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

@lianga888 lianga888 deleted the schedule_function_resilience branch Jun 9, 2016

jbredeche added a commit that referenced this pull request Jun 9, 2016

Revert "Merge pull request #1226 from quantopian/schedule_function_re…
…silience"

This reverts commit 6079604, reversing
changes made to c9b5979.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment