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

MAINT: Add equity to naming of bar data classes. #1311

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

ehebert
Copy link
Contributor

@ehebert ehebert commented Jun 29, 2016

In preparation of adding futures, add equity to the names of both the
classes and methods for writing bcolz data. Futures data will use a
different minutes per day with a separate reader. This change will allow
both equity and futures fixtures to be side by side.

Also, break out the method which generates the dataframes and trading
days member into fixtures (EquityMinuteBarData and
EquityDailyBarData) on which the *BarReader fixture depends. This
fixture is separated out to enable reader/writers in different formats
to use the same data setup. (There is internal code which needs to write
minute and daily bar data in a database format.)

@ehebert
Copy link
Contributor Author

ehebert commented Jun 29, 2016

cc: @llllllllll @jbredeche @yankees714

@yankees714 is this compatible with changes you are making to the minute bar readers? Let me know what I can do to make this collide too much with your work.

Once this is greenlit, I will do the appropriate replacement of class and method names in our internal repos.

@ehebert
Copy link
Contributor Author

ehebert commented Jun 29, 2016

Just noticed this name class WithEquityBcolzEquityDailyBarReaderFromCSVs( WithEquityBcolzEquityDailyBarReader):; fixing.

@ehebert ehebert force-pushed the prefix-equity-bar-fixtures branch from 45525bc to 9b3538f Compare June 29, 2016 04:34
@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage increased (+0.01%) to 82.998% when pulling 9b3538f on prefix-equity-bar-fixtures into c89e957 on master.

@ehebert
Copy link
Contributor Author

ehebert commented Jun 29, 2016

#1311 (comment) was addressed with a fixup.

@@ -636,7 +636,7 @@ def convert_col(col):
table.flush()


class BcolzMinuteBarReader(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

will the future reader be a different class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked offline with @llllllllll. Going to roll back the part of this PR which creates separate reader classes, in favor of one class which uses parameters. The WithBcolzEquityMinuteBarReader and WithBcolzFutureMinuteBarReader fixtures will be responsible for setting the respective minutes per day values.

@ehebert ehebert force-pushed the prefix-equity-bar-fixtures branch from 9b3538f to f286518 Compare June 29, 2016 19:47
@ehebert
Copy link
Contributor Author

ehebert commented Jun 29, 2016

Pushed fixups addressing all comments. Going to merge once checks pass again, and a corresponding internal branch has been crafted.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage increased (+0.02%) to 83.0% when pulling f286518 on prefix-equity-bar-fixtures into c89e957 on master.

1 similar comment
@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage increased (+0.02%) to 83.0% when pulling f286518 on prefix-equity-bar-fixtures into c89e957 on master.

In preparation of adding futures, add equity to the names of both the
classes and methods for writing bcolz data. Futures data will use a
different minutes per day with a separate reader. This change will allow
both equity and futures fixtures to be side by side.

Also, break out the method which generates the dataframes and trading
days member into fixtures (`EquityMinuteBarData` and
`EquityDailyBarData`) on which the `*BarReader` fixture depends.  This
fixture is separated out to enable reader/writers in different formats
to use the same data setup. (There is internal code which needs to write
minute and daily bar data in a database format.)
@ehebert ehebert force-pushed the prefix-equity-bar-fixtures branch from f286518 to 51eda06 Compare June 30, 2016 12:21
@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage increased (+0.02%) to 83.0% when pulling 51eda06 on prefix-equity-bar-fixtures into c89e957 on master.

@ehebert ehebert merged commit 65a1e34 into master Jun 30, 2016
@ehebert ehebert deleted the prefix-equity-bar-fixtures branch June 30, 2016 13:55
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.

3 participants