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

BUG: factor_rank_autocorrelation infers turnover period in calendar... #324

Merged
merged 2 commits into from Nov 3, 2018

Conversation

luca-s
Copy link
Collaborator

@luca-s luca-s commented Nov 3, 2018

…space while periods could have different time space

quantile_turnover is affected by the same bug

closes Issue #323

…pace

...while periods could have different time space

quantile_turnover is affected by the same bug

Issue quantopian#323
@twiecki
Copy link
Contributor

twiecki commented Nov 4, 2018

So this removes the ability to pass in anything more fine-grained than days? That's quite a hefty price-tag for fixing that edge case.

@luca-s
Copy link
Collaborator Author

luca-s commented Nov 4, 2018

No, don't worry. I didn't change the Alphalens behaviour. Due to the code change I noticed the are few test cases, which I created some time ago, that fail but also they don't make sense. Since it took some time to figure that out, I made sure the functions in utils reise now an exception when they receive a wrong parameter type.

@luca-s
Copy link
Collaborator Author

luca-s commented Nov 4, 2018

Just to be more detailed, the three tests were wrong for the following reason:

  • freq represents the factor trading calendar, which can be only Day, BusinessDay or CustomBusinessDay
  • the factor data itself can have any frequency, even hours, minutes, seconds. The period at which we analyse the factor can use these frequencies even though the trading calendar has daily resolution
  • the tests that I removed used the factor data frequency (10min, 1 hour and 3 hour) as value for freq, which is wrong
    So I deleted the wrong tests but we can still analyse a factor at any frequency we like.

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.

None yet

2 participants