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

Proposal: change period/annualization API #20

Open
twiecki opened this issue Aug 23, 2016 · 6 comments
Open

Proposal: change period/annualization API #20

twiecki opened this issue Aug 23, 2016 · 6 comments

Comments

@twiecki
Copy link
Contributor

twiecki commented Aug 23, 2016

One thing I currently think is suboptimal is that we provide period and annualization to do the same thing. That might not be problematic if it was for a broad use-case but most users will be zipline or quantopian users and pass in daily returns. Two alternatives:

  • Set global vars like DAILY = 252 and have only annualization and allow it to be passed annualization=DAILY or annualization=356 if someone wants custom.
  • Allow annualization to also be a str like 'daily', essentially move period to annualization.
@twiecki
Copy link
Contributor Author

twiecki commented Aug 23, 2016

CC @ahgnaw @abhijeetkalyan

@analicia
Copy link
Contributor

Great idea, that way we can simplify the API.

@twiecki
Copy link
Contributor Author

twiecki commented Aug 23, 2016

Glad you like it, what do you think of the two options? I kinda like the first one for its simplicity, but the other one has it's pros too.

@ssanderson
Copy link

ssanderson commented Aug 23, 2016

I would be careful about enshrining a canonical definition of what "daily" means. There are ~252 trading days in the NYSE calendar, but Zipline already supports calendars other than NYSE, which have different numbers of trading days.

@analicia
Copy link
Contributor

I have a preference towards the second one. Instead of having users write empyrical.function(returns, empyrical.DAILY) they could write empyrical.function(returns, 'daily')

@twiecki
Copy link
Contributor Author

twiecki commented Aug 24, 2016

@ssanderson Since it's a free parameter users can pass in whatever is appropriate for the exchange they are trading. But we should definitely make that clear in the doc-string that the default is relative to NYSE.
@ahgnaw Yep, that's fine with me.

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

No branches or pull requests

3 participants