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: Correct AverageDollarVolume NaN handling #1309

Merged
merged 5 commits into from Jun 30, 2016

Conversation

Projects
None yet
5 participants
@nathanwolfe
Contributor

nathanwolfe commented Jun 28, 2016

AverageDollarVolume used nanmean, which discards NaNs before
averaging, giving an ADV which is too high for any equities that have
any NaNs.

Changing the method to nansum divided by window length so that the
denominator is the same no matter whether there are NaNs or not.

BUG: Correct AverageDollarVolume NaN handling
`AverageDollarVolume` used `nanmean`, which discards NaNs before
averaging, giving an ADV which is too high for any equities that have
any NaNs.

Changing the method to `nansum` divided by window length so that the
denominator is the same no matter whether there are NaNs or not.
@richafrank

This comment has been minimized.

Member

richafrank commented Jun 28, 2016

Nice! Should we add tests for this case?

@nathanwolfe

This comment has been minimized.

Contributor

nathanwolfe commented Jun 29, 2016

Added tests. Old AverageDollarVolume fails, new one passes.

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 82.984% when pulling ebbcca7 on nathanwolfe:adv-fix into c89e957 on quantopian:master.

@@ -1056,7 +1056,12 @@ def init_class_fixtures(cls):
index=dates,
columns=cls.asset_finder.retrieve_all(sids),
)
cls.raw_data_with_nans = cls.raw_data.where(cls.raw_data % 3 != 0)

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 29, 2016

Contributor

I would probably write this as ((cls.raw_data % 3) != 0) for clarity

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 29, 2016

Contributor

Actually, I would also use % 2 here, because otherwise this data is just all NaNs for the first asset and all filled values for the others. This doesn't test the case of having some NaNs and some values.

@@ -1207,6 +1221,15 @@ def test_dollar_volume(self):
expected_5 = rolling_mean((self.raw_data ** 2) * 2, window=5)[5:]
assert_frame_equal(results['dv5'].unstack(), expected_5)
expected_1_nan = (self.raw_data_with_nans[5:]
* self.raw_data[5:] * 2).fillna(0)

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 29, 2016

Contributor

You can still just do (self.raw_data_with_nans ** 2) * 2 here. Multiplying by self.raw_data will just fill with NaNs in the same places.

This comment has been minimized.

@nathanwolfe

nathanwolfe Jun 29, 2016

Contributor

Is there any reason to make that change? Not squaring reflects that USEquityPricing.open and USEquityPricing.volume don't both have the NaNs.

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 29, 2016

Contributor

Good point, I would just comment that in then

assert_frame_equal(results['dv1_nan'].unstack(), expected_1_nan)
expected_5_nan = rolling_mean((self.raw_data_with_nans
* self.raw_data * 2).fillna(0),

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 29, 2016

Contributor

Same as above

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 82.984% when pulling 985e6ba on nathanwolfe:adv-fix into c89e957 on quantopian:master.

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Jun 29, 2016

Looks good to me, unless @llllllllll has anything

@richafrank

This comment has been minimized.

Member

richafrank commented Jun 29, 2016

This fix also seems worthy of the release notes - would you add an entry there?

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Jun 29, 2016

@richafrank Is that appveyor failure intermittent? I'm not seeing any logs

@richafrank

This comment has been minimized.

Member

richafrank commented Jun 29, 2016

@richafrank Is that appveyor failure intermittent? I'm not seeing any logs

That's my guess. We can see on the next update...

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 82.984% when pulling e0e18bc on nathanwolfe:adv-fix into c89e957 on quantopian:master.

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Jun 29, 2016

We can see on the next update...

Looks like it was

@dmichalowicz dmichalowicz merged commit d6c1c5f into quantopian:master Jun 30, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment