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

Schedule function updates #411

Merged
merged 0 commits into from Oct 20, 2014

Conversation

Projects
None yet
4 participants
@llllllllll
Member

llllllllll commented Oct 13, 2014

No description provided.

@llllllllll llllllllll force-pushed the schedule-function-runtime-errors branch 2 times, most recently from 525befe to 7e95e8e Oct 13, 2014

@llllllllll llllllllll changed the title from Schedule function runtime errors to Schedule function updates Oct 13, 2014

@jbredeche

This comment has been minimized.

Member

jbredeche commented Oct 15, 2014

@llllllllll can I see screenshots of the various error messages in the UI now?

@llllllllll llllllllll force-pushed the schedule-function-runtime-errors branch from 5db1da0 to 7463c7a Oct 15, 2014

@llllllllll

This comment has been minimized.

Member

llllllllll commented Oct 15, 2014

month-error
time-error
week-error

@coveralls

This comment has been minimized.

coveralls commented Oct 15, 2014

Coverage Status

Coverage decreased (-0.28%) when pulling 7463c7a on schedule-function-runtime-errors into 50c5b73 on master.

@@ -74,6 +74,11 @@ def test_build_offset_both(self):
with self.assertRaises(ValueError):
_build_offset(datetime.timedelta(minutes=1), {'minutes': 1}, None)
def test_build_offset_exc(self):

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

What's the reason for having this test?

This comment has been minimized.

@llllllllll

llllllllll Oct 15, 2014

Member

For more test coverage of what I could imagine will be a semi-common code path.

@@ -301,7 +306,7 @@ def test_NDaysBeforeLastTradingDayOfWeek(self, n):
self.assertEqual(n_tdays, n)
@parameterized.expand(param_range(30))
@parameterized.expand(param_range(26))

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

I'm assuming this is 26 because that's the most trading days a month can have. How do we handle an offset of 26 if the month doesn't have 26 trading days?

This comment has been minimized.

@llllllllll

llllllllll Oct 15, 2014

Member

That follows the same rules as range, so it is exclusive on the 26; however, if you tried to trade on the 25th day and the given month had a holiday or something then it would not trade that month. This comes into question of what is more important, The proper way to get the last day would be to give an offset from (month|week)_end.

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

We use 26 in a bunch of places in this PR. It should probably be a module-level constant (MAX_DAYS_OFFSET or somesuch).

This comment has been minimized.

@llllllllll

llllllllll Oct 15, 2014

Member

I will do the same with MAX_DAY_OFFSET = 5

@@ -66,6 +69,55 @@ def ensure_utc(time, tz='UTC'):
return time.replace(tzinfo=pytz.utc)
def _coerce_datetime(maybe_dt):

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

I like this function.

)
_TD_CHECK_DELTA = datetime.timedelta(hours=6, minutes=30)

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

This should probably just live inside _td_check_delta. It's not used anywhere else, and that function has a hard-coded error message that references the value of this constant.

This comment has been minimized.

@llllllllll

llllllllll Oct 15, 2014

Member

kk; however, I was worried about constructing a new timedelta object every time I wanted to make this check.

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

If that's a concern the post efficient way to do this is probably if td.total_seconds() > 23400 (with a comment saying that that's the magnitude of a 6:30 delta.

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

also, I think this fails on negative timedeltas?

This comment has been minimized.

@llllllllll

llllllllll Oct 15, 2014

Member

I like the seconds + comment idea. Also, isn't a timedelta a distance between two points in time thus always positive?

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member
In [12]: pd.Timestamp('2013-01-01') + timedelta(days=-1)
Out[12]: Timestamp('2012-12-31 00:00:00', tz=None)
@@ -356,8 +410,8 @@ class NthTradingDayOfMonth(StatelessRule):
This is zero-indexed, n=0 is the first trading day of the month.
"""
def __init__(self, n=0):
if n not in range(31):
raise ValueError('n must be in [0,31)')
if n not in range(26):

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

This should be if n < 0 or n >= 26.

This comment has been minimized.

@llllllllll

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

Or rather, it shouldn't be constructing a length-25 list and doing a contains check. Exactly how you want to fix that is up to you.

@llllllllll llllllllll force-pushed the schedule-function-runtime-errors branch 2 times, most recently from 5543965 to ffa54c4 Oct 15, 2014

@coveralls

This comment has been minimized.

coveralls commented Oct 15, 2014

Coverage Status

Coverage decreased (-0.29%) when pulling ffa54c4 on schedule-function-runtime-errors into 50c5b73 on master.

if 0 <= seconds <= 23400:
return td
else:
raise ValueError('offset cannot exceed 6 hours and 30 minutes')

This comment has been minimized.

@ssanderson

ssanderson Oct 15, 2014

Member

Might want a separate error message for less than 0.

@llllllllll llllllllll force-pushed the schedule-function-runtime-errors branch from ffa54c4 to ab31668 Oct 16, 2014

self.td_delta,
self.get_last_trading_day_of_week(dt),
) == dt.date()
)).date() == dt.date()

This comment has been minimized.

@ssanderson

ssanderson Oct 16, 2014

Member

Note to self and @ehebert: this is WAY faster than normalizing with pandas and comparing.

In [6]: %%timeit
   ...: x.date() == y.date()
   ...: 
The slowest run took 8.96 times longer than the fastest. This could mean that an intermediate result is being cached 
1000000 loops, best of 3: 426 ns per loop

In [7]: %%timeit
pd.tseries.tools.normalize_date(x) == pd.tseries.tools.normalize_date(y)
   ...: 
The slowest run took 6.66 times longer than the fastest. This could mean that an intermediate result is being cached 
100000 loops, best of 3: 4.83 µs per loop
@ssanderson

This comment has been minimized.

Member

ssanderson commented Oct 16, 2014

This looks good to me.

@llllllllll llllllllll merged commit c6e85d0 into master Oct 20, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@llllllllll llllllllll force-pushed the schedule-function-runtime-errors branch from ab31668 to c6e85d0 Oct 20, 2014

@llllllllll llllllllll deleted the schedule-function-runtime-errors branch Oct 20, 2014

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