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

Allow for different periodicity (annualization factors) in the annual_*() methods #164

Closed
wants to merge 11 commits into from

Conversation

cgdeboer
Copy link
Contributor

@cgdeboer cgdeboer commented Oct 7, 2015

Summary

I have a use case for passing in a Series of monthly returns to a variety of the methods available in timeseries.py. A lot of the methods will work and calculate properly regardless of the periodicity of the data. Things like cum_returns, normalize, max_drawdown all are frequency agnostic. I've added a 'period' kwarg to the annual_return and annual_volatility function to allow for the specification of other frequencies (the default remains daily).

Changes

  • added some str defaults in .utils.py to help with possible clerical errors in reusing the terms, daily, weekly, monthly and yearly.
  • added a dict constant in utils.py that specifies the annualization factor based on frequency.
  • annual_return and annual_volatility now raises a ValueError if you pass in a 'period' that is not accepted (followed the precedent set in the aggregate_returns() method.

    NOTE: I noticed that the aggregate_returns method does not actually raise the ValueError that is set in the last line. I left that untouched, but I think it should be python raise ValueError("....")

Notes

There is certainly more one could do to support frequencies app-wide, as some raw return data is not available at the daily periodicity level, however, I didn't want to short-circuit or prematurely jumpstart any plans you all had to begin that effort.

@twiecki twiecki self-assigned this Oct 8, 2015
@twiecki
Copy link
Contributor

twiecki commented Oct 8, 2015

Thanks @cgdeboer! I'll try to take a look soon.

ann_factor = ANNUALIZATION_FACTORS[period]
except KeyError:
raise ValueError(
"periodicty must cannot be: '{}'."
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@justinlent
Copy link
Contributor

@cgdeboer Thanks for this PR! It looks really nice -- very clean implementation, and very readable code. I have one concern in that when migrating from one periodicity (e.g. Daily) to supporting various ones with various annualization factors, etc., I feel like its possible that things can break pretty easily downstream (edge cases, etc). I'd feel more comfortable merging this PR after some unit tests. We already have a bunch for our existing metrics, perhaps you can add a couple of test conditions to the existing unit tests?

Thanks again!

@cgdeboer
Copy link
Contributor Author

@twiecki and @justinlent , thanks for the feedback. I added a couple tests (just for the two primary annualization methods) and also ensured that other functions that were calling the annual_return and annual_volatility methods are have an optional kwarg for period.

A couple notes. I toyed with the idea of using pandas.infer_freq or Series.index.freqstr to sort of "autodetect" the frequency of the data being passed in. That would seem more optimal to me; it beats passing in an addtional kwarg to every method. As I was doing some local testing on my idea, the pandas.index.freq support is not as robust as I thought. Weird things happen when the series is not exactly daily, or if you have an Index that is not a `DateTimeIndex`` yet consists of perfect calendar month end dates. Sure the user could convert to a DateTimeSeries first as a requirement, but in this case it seemed better for the time being to allow the user to tell the function explicitly what frequency to use, rather than it being "magic". Perhaps that can be refactored later to have the method guess the frequency, but allow the user to override with a kwarg.

Let me know if I need to add anything else to the code or tests.

@cgdeboer
Copy link
Contributor Author

Looks like one of the tests is failing now....I will investigate later this weekend. Hang tight.

@cgdeboer
Copy link
Contributor Author

Hi guys, not sure why the build is failing at this point; I'm unable to replicate this far on running tests locally in a new virtualenv. Can you point me in the right direction on this? The error traces back to a numpy import which seems irrelevant to what I was working on.

https://travis-ci.org/quantopian/pyfolio/jobs/84695009

@twiecki
Copy link
Contributor

twiecki commented Oct 12, 2015

That error is very odd, hard to believe it has anything to do with what you did. I restarted the build just in case.

@twiecki
Copy link
Contributor

twiecki commented Oct 14, 2015

Merged with ee928a3. Many thanks!

@twiecki twiecki closed this Oct 14, 2015
@cgdeboer cgdeboer mentioned this pull request Oct 14, 2015
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