Skip to content

Conversation

llllllllll
Copy link
Contributor

No description provided.

Copy link
Contributor

@ssanderson ssanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty painless! I had a couple requests for more test coverage and clarifications in docstrings, and one question about timezone expectations.

Returns
-------
data_query_cutoff : pd.DatetimeIndex
The sessions at the given data query time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"At the given query time" is a bit confusing since there's no cutoff time being provided to this function.


def data_query_cutoff_for_sessions(self, sessions):
return days_at_time(
sessions.tz_localize('UTC'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sessions always expected to be tz-naive? I would have expected our normal midnight UTC convention here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the tests, it appears to never be localized, meaning we are passing unlocalized dts to pipeline loaders.

Name of the calendar, to be looked by by trading_calendar.get_calendar.
data_query_offset : datetime.timedelta
The offset from market open when data should no longer be considered
available for a session.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example might be helpful here, e.g. "For example, a data_query_offset of -timedelta(minutes=45) signifies that data must have been available 45 minutes prior to market open for it to appear in pipeline inputs for a given day."

@llllllllll llllllllll force-pushed the data-query-time branch 2 times, most recently from 787d5d2 to f0e75cf Compare September 10, 2018 20:35
@llllllllll llllllllll changed the base branch from international-pipelines to master September 10, 2018 20:36
@llllllllll llllllllll closed this Sep 13, 2018
@llllllllll llllllllll reopened this Sep 13, 2018
Copy link
Contributor

@ssanderson ssanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I had a few questions, and I posted a few updates to the tests in commits at the end of this PR.

@ssanderson ssanderson merged commit 9363f03 into master Sep 23, 2018
@ssanderson ssanderson deleted the data-query-time branch September 23, 2018 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants