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

PERF: Avoid repeated recursive calls when getting forward-filled close #1735

Merged
merged 1 commit into from Apr 6, 2017

Conversation

Projects
None yet
4 participants
@yankees714
Contributor

yankees714 commented Apr 3, 2017

Instead of recursively calling DailyHistoryAggregator.closes until we find a non-nan close, we can instead call load_raw_arrays once, and find the value from the returned array.

@yankees714 yankees714 requested a review from ehebert Apr 3, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 3, 2017

Coverage Status

Coverage increased (+0.005%) to 87.469% when pulling f4e91f8 on speedup-daily-history-aggregator-closes into a006b4b on master.

@ehebert

This comment has been minimized.

Member

ehebert commented Apr 4, 2017

@yankees714 do you have profiles/timings of this change using a contract which fits in the happy case (i.e. the last minute of the day is not nan.) as well as the worst case that this is fixing?
(I am slightly worried about incurring cost of creating arrays in cases that they are not needed.)

Depending on those results, we may want to have the logic assume best case and get the last value, and then assume the worst if it is nan.

@yankees714

This comment has been minimized.

Contributor

yankees714 commented Apr 5, 2017

Think I may have mis-interpreted @ehebert's question here at first - the case where the minute passed to closes() has a non-nan close should not be effected. We do see a bit of a slowdown where the value is nan and the value for the previous minute is non-nan (~180 µs -> ~220 µs), but we don't think this is significant enough to add extra logic.

@yankees714 yankees714 force-pushed the speedup-daily-history-aggregator-closes branch from f4e91f8 to 7b4a15b Apr 5, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 5, 2017

Coverage Status

Coverage increased (+0.005%) to 87.519% when pulling 7b4a15b on speedup-daily-history-aggregator-closes into 8a672be on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 5, 2017

Coverage Status

Coverage increased (+0.005%) to 87.519% when pulling a4c69af on speedup-daily-history-aggregator-closes into 8a672be on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Apr 5, 2017

Coverage Status

Coverage increased (+0.005%) to 87.519% when pulling a4c69af on speedup-daily-history-aggregator-closes into 8a672be on master.

TRADING_ENV_MIN_DATE = START_DATE = pd.Timestamp(
'2016-03-01', tz='UTC',
)
TRADING_ENV_MAX_DATE = END_DATE = pd.Timestamp(

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 5, 2017

Contributor

I don't think TRADING_ENV_MIN_DATE and TRADING_ENV_MAX_DATE are necessary here (the test works without them)

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Apr 5, 2017

@yankees714 The new test looks good to me, though it would be nice to be able to share a lot of that code. What do you think of moving the test into the existing MinuteToDailyAggregationTestCase suite and creating a shared helper function? Also, is there any way to use this test to compare the old behavior to the new? (i.e. does it fail or is it just slower, in which case I'm not immediately sure how to compare)

@yankees714

This comment has been minimized.

Contributor

yankees714 commented Apr 6, 2017

@dmichalowicz Good call on sharing a helper function - I was initially tentative to include both the futures and equities setup in the same test case, but I like the way it came out.

For the behavior comparison, the new test fails on master (with the max recursion depth exceeded issue).

@coveralls

This comment has been minimized.

coveralls commented Apr 6, 2017

Coverage Status

Coverage increased (+0.005%) to 87.519% when pulling 8cafb69 on speedup-daily-history-aggregator-closes into 8a672be on master.

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Apr 6, 2017

The shared test looks great, nice work. This PR looks good by me, unless @ehebert had any other comments

PERF: Avoid repeated recursive calls when getting forward-filled close
Instead of recursively calling `DailyHistoryAggregator.closes` until we
find a non-nan close, we can instead call `load_raw_arrays` once, and
find the value from the returned array.

@yankees714 yankees714 force-pushed the speedup-daily-history-aggregator-closes branch from 8cafb69 to f4f2048 Apr 6, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 6, 2017

Coverage Status

Coverage increased (+0.005%) to 87.519% when pulling f4f2048 on speedup-daily-history-aggregator-closes into 8a672be on master.

@yankees714 yankees714 merged commit ae1f9f8 into master Apr 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment