Skip to content
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

BUG: get_last_traded_dt expects a trading day #2087

Merged
merged 1 commit into from Jan 23, 2018
Merged

Conversation

@richafrank
Copy link
Member

@richafrank richafrank commented Jan 22, 2018

Otherwise we get back NaN.

The consumer of history_start is the later call to self.get_last_traded_dt, which returns NaN for non-trading days.

We were so close to catching this in the previous PR...

@ehebert @yankees714 Is DataPortal.trading_calendar good enough here, or do we need to look up a calendar per asset (and/or per reader)?

@richafrank richafrank requested review from ehebert and yankees714 Jan 22, 2018
np.testing.assert_allclose(
window[-1],
expected,
err_msg="at minute {}".format(minute),

This comment has been minimized.

@richafrank

richafrank Jan 22, 2018
Author Member

These changes to the assertions are just adding more informative messages.

@richafrank richafrank force-pushed the still-trying-to-ffill branch from ffd29d9 to ae619aa Jan 22, 2018
@coveralls
Copy link

@coveralls coveralls commented Jan 22, 2018

Coverage Status

Coverage remained the same at 87.579% when pulling ae619aa on still-trying-to-ffill into 32c0747 on master.

@coveralls
Copy link

@coveralls coveralls commented Jan 22, 2018

Coverage Status

Coverage remained the same at 87.579% when pulling 48a66ce on still-trying-to-ffill into 32c0747 on master.

@ehebert
Copy link
Contributor

@ehebert ehebert commented Jan 22, 2018

The reader calendars are aligned to the DataPortal.trading_calendar, so I believe that your use of the trading_calendar member is correct.

@@ -1600,7 +1600,8 @@ def test_daily_history_minute_gaps_price_ffill(self, test_name, bar_count):
# January 5 2015 is the first day, and there is volume only every
# 10 minutes.
for day_idx, day in enumerate([pd.Timestamp('2015-01-05', tz='UTC'),
pd.Timestamp('2015-01-06', tz='UTC')]):
pd.Timestamp('2015-01-06', tz='UTC'),
pd.Timestamp('2015-01-12', tz='UTC')]):

This comment has been minimized.

@ehebert

ehebert Jan 22, 2018
Contributor

This date is crafted to include a day for which the previous day (when using Timedelta) is not a trading day, correct?
Might want to put a comment here about that, so than any future edits do not lose the intent of the test.

This comment has been minimized.

@richafrank

richafrank Jan 22, 2018
Author Member

Yes - good call.

Copy link
Contributor

@ehebert ehebert left a comment

LGTM.

@richafrank richafrank force-pushed the still-trying-to-ffill branch from ae619aa to 32a2239 Jan 22, 2018
Otherwise we get back NaN.
@richafrank richafrank force-pushed the still-trying-to-ffill branch from 32a2239 to 48a66ce Jan 22, 2018
@richafrank richafrank merged commit fe5db72 into master Jan 23, 2018
2 checks passed
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 still-trying-to-ffill branch Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.