这是indexloc提供的服务,不要输入任何密码
Skip to content

ENH: Add default index names to pipeline results #1997

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

analicia
Copy link
Contributor

Improve legibility of pipeline result manipulations by adding names to the multi-index.

@coveralls
Copy link

coveralls commented Oct 27, 2017

Coverage Status

Coverage increased (+0.003%) to 87.224% when pulling 6ef43dc on add-pipeline-result-index-names into 61c8869 on master.

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 fine to me. I think ideally we'd have consistent pluralization of the column names, but my gut is that that consistency isn't worth breaking all the downstream code that's using those fields.

Do you have a sense of how much breakage we'd be taking on if we changed the names?

@@ -32,6 +32,11 @@
from zipline.utils.sharedoc import copydoc


DATE_INDEX_NAME = 'dates'
Copy link
Contributor

Choose a reason for hiding this comment

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

It bothers me that dates is plural here but sid is singular. It's probably not worth breaking all the consumers of this to make the names consistent though :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to sids. All of the downstream consumers currently change the name of the index from (None, None), to (dates, sid).

@@ -32,6 +32,11 @@
from zipline.utils.sharedoc import copydoc


DATE_INDEX_NAME = 'dates'
SID_INDEX_NAME = 'sid'
PIPELINE_INDEX_NAMES = (DATE_INDEX_NAME, SID_INDEX_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably put these in zipline.pipeline.common instead of in engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, moving them.

@analicia analicia force-pushed the add-pipeline-result-index-names branch from 6ef43dc to dbf1305 Compare November 14, 2017 23:56
@coveralls
Copy link

coveralls commented Nov 15, 2017

Coverage Status

Coverage increased (+0.002%) to 87.322% when pulling dbf1305 on add-pipeline-result-index-names into 28a41a5 on master.

@analicia analicia force-pushed the add-pipeline-result-index-names branch from dbf1305 to f0af00d Compare November 27, 2017 18:18
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 87.343% when pulling f0af00d on add-pipeline-result-index-names into 079ea3d on master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 27, 2017

Coverage Status

Coverage increased (+0.002%) to 87.343% when pulling f0af00d on add-pipeline-result-index-names into 079ea3d on master.

@analicia analicia force-pushed the add-pipeline-result-index-names branch from f0af00d to ce7b654 Compare November 29, 2017 23:06
@coveralls
Copy link

coveralls commented Nov 29, 2017

Coverage Status

Coverage increased (+0.002%) to 87.374% when pulling ce7b654 on add-pipeline-result-index-names into 49a3a4a on master.

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