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

bugfix: convert index to int so that the pandas indexing doesnt fail #1683

Merged

Conversation

@shadiakiki1986
Copy link
Contributor

@shadiakiki1986 shadiakiki1986 commented Feb 13, 2017

otherwise, the index could come out as a float because of the division ahead of it

@coveralls
Copy link

@coveralls coveralls commented Feb 13, 2017

Coverage Status

Coverage increased (+0.0009%) to 87.412% when pulling eb748c7 on shadiakiki1986:bugfix_index_should_be_int into 296307a on quantopian:master.

@freddiev4
Copy link
Contributor

@freddiev4 freddiev4 commented Mar 6, 2017

Hi @shadiakiki1986 thanks for the PR. I was wondering if you could explain/show some test where this is an issue, so I could try and reproduce it, or how you caught this

@shadiakiki1986
Copy link
Contributor Author

@shadiakiki1986 shadiakiki1986 commented Mar 6, 2017

Hello. I'm wrapping zipline's blotter class in a django application here. I ran into this issue by pure coincidence. The division yielded a float. I'm not sure where exactly in my code it was triggered, but it might be here


# cast to int for pandas
# https://github.com/pandas-dev/pandas/blob/master/pandas/tseries/base.py#L247
num_days = int(num_days)

This comment has been minimized.

@freddiev4

freddiev4 Mar 7, 2017
Contributor

I think that if we're going to check for an int, we should be doing this earlier, before we even check for num_days == 0

We can do that on line 552; using the // operator which does integer division in both Python 2 and 3: num_days = data['shape'][0] // self._minutes_per_day

This comment has been minimized.

@shadiakiki1986

shadiakiki1986 Mar 8, 2017
Author Contributor

That's a better idea. I just pushed a new commit amending the PR

@@ -549,7 +549,9 @@ def last_date_in_output_for_sid(self, sid):
with open(sizes_path, mode='r') as f:
sizes = f.read()
data = json.loads(sizes)
num_days = data['shape'][0] / self._minutes_per_day
# use integer division so that the result is an int for pandas index later
# https://github.com/pandas-dev/pandas/blob/master/pandas/tseries/base.py#L247

This comment has been minimized.

@freddiev4

freddiev4 Mar 8, 2017
Contributor

Looks like the flake8 tests failed because line 552 is too long (and probably 553 as well)

This comment has been minimized.

@shadiakiki1986

shadiakiki1986 Mar 8, 2017
Author Contributor

shortened the lines

@coveralls
Copy link

@coveralls coveralls commented Mar 8, 2017

Coverage Status

Coverage increased (+0.001%) to 87.413% when pulling e29aeb3 on shadiakiki1986:bugfix_index_should_be_int into 296307a on quantopian:master.

@@ -549,7 +549,9 @@ def last_date_in_output_for_sid(self, sid):
with open(sizes_path, mode='r') as f:
sizes = f.read()
data = json.loads(sizes)
num_days = data['shape'][0] / self._minutes_per_day
# use integer division so that the result is an int
# for pandas index later https://goo.gl/VAW649

This comment has been minimized.

@freddiev4

freddiev4 Mar 8, 2017
Contributor

Ok great. Just one last thing; we should keep this link expanded (so people know it's a safe link to click). You can make the flake8 tests ignore the length of this line by adding # noqa line 553. So it would look like:

# for pandas index later <link> # noqa

Should've mentioned that before, sorry! (after that it should be good to merge)

This comment has been minimized.

@freddiev4

freddiev4 Mar 8, 2017
Contributor

Also you should squash your commits into 1, and change the commit message to say BUG: <description> (to be consistent with our dev guidelines https://github.com/quantopian/zipline/wiki/Zipline-development-guidelines)

This comment has been minimized.

@shadiakiki1986

shadiakiki1986 Mar 13, 2017
Author Contributor

No problem :) Done and rebased

@shadiakiki1986 shadiakiki1986 force-pushed the shadiakiki1986:bugfix_index_should_be_int branch from e29aeb3 to 045d95f Mar 13, 2017
@shadiakiki1986 shadiakiki1986 force-pushed the shadiakiki1986:bugfix_index_should_be_int branch from 045d95f to 5bea92f Mar 13, 2017
@coveralls
Copy link

@coveralls coveralls commented Mar 13, 2017

Coverage Status

Coverage remained the same at 87.474% when pulling 5bea92f on shadiakiki1986:bugfix_index_should_be_int into 40aec72 on quantopian:master.

@coveralls
Copy link

@coveralls coveralls commented Mar 13, 2017

Coverage Status

Coverage remained the same at 87.474% when pulling 5bea92f on shadiakiki1986:bugfix_index_should_be_int into 40aec72 on quantopian:master.

@shadiakiki1986
Copy link
Contributor Author

@shadiakiki1986 shadiakiki1986 commented Mar 16, 2017

Btw, here is my travis build which demonstrates this issue

@freddiev4
Copy link
Contributor

@freddiev4 freddiev4 commented Mar 16, 2017

Looks good to merge 👍 thanks for the PR

@freddiev4 freddiev4 merged commit 5fd20e1 into quantopian:master Mar 16, 2017
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
@shadiakiki1986 shadiakiki1986 deleted the shadiakiki1986:bugfix_index_should_be_int branch Mar 17, 2017
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.