ranaroussi / quantstats

Portfolio analytics for quants, written in Python
Apache License 2.0
4.85k stars 846 forks source link

CAGR % Calculation in reports.html #275

Open glformanek opened 1 year ago

glformanek commented 1 year ago

The call to calculate the CAGR should allow you to use the periods_per_year in the calculation or use a different way to compute it. The CAGR formula is: CAGR = (((Ending Value / Beginning Value) ^ (1 / # of years)) - 1) * 100

​The # of years could be calculated by taking the ending date - beginning date which gives you the days. Then divide the days by 365 to get the years.

Some references: https://www.investopedia.com/terms/c/cagr.asp#:~:text=To%20calculate%20the%20CAGR%20of,one%20from%20the%20subsequent%20result https://seekingalpha.com/article/4516791-compound-annual-growth-rate-cagr#cagr-formula

This is the statement that does the calculation and seems to be incorrect (I think it should use 365): metrics["CAGR﹪%"] = _stats.cagr(df, rf, compounded) * pct

Just briefly looking, it could be done by hard coding 365 or perhaps using the below change: metrics["CAGR﹪%"] = _stats.cagr(df, rf, compounded, periods=periods_per_year) * pct

glformanek commented 1 year ago

Another possible fix would be to have the cagr funtion default to periods=365. This would line up with the references noted above. This would also be a simple change.

gnzsnz commented 1 year ago

this looks like a duplicate of 273

@glformanek what would be the rational for having periods=365?

I have fixed the issue on my local repository by simply removing the periods parameters. periods is only used to calculate the number of years.

years = (returns.index[-1] - returns.index[0]).days / periods

The thing is that i cant think of a case that requires periods different than 365.

this piece of code (returns.index[-1] - returns.index[0]).days will always returns days (unless the index is not in date format, which will just fail) and once you have this in days the only thing that will work to calculate CAGR is 365. But i might be missing something.

glformanek commented 1 year ago

You're right about some of this being a duplicate (CAGR calculation). But in issue 273, there is also the rebalance issue when you leave the default value (function defaults to rebalance='1M'. It wont' work with the default or a different time period. Also, I saw the proposed fix to CAGR but I think the best way to fix it is to fix the CAGR function to default to periods=365. That is a simple fix and you don't have to mess with anything in the reports.py. Just a suggestion. I agree that I've only seen CAGR calculated with 365, so I'm not sure there is a need for the periods parameter but I don't know the rest of the code.

Regards

On Sat, Jul 15, 2023 at 3:52 AM gnzsnz @.***> wrote:

this looks like a duplicate of 273 https://github.com/ranaroussi/quantstats/issues/273

@glformanek https://github.com/glformanek what would be the rational for having periods=365?

I have fixed the issue on my local repository by simply removing the periods parameters. periods is only used to calculate the number of years.

years = (returns.index[-1] - returns.index[0]).days / periods

The thing is that i cant think of a case that requires periods different than 365.

this piece of code (returns.index[-1] - returns.index[0]).days will always returns days (unless the index is not in date format, which will just fail) and once you have this in days the only thing that will work to calculate CAGR is 365. But i might be missing something.

— Reply to this email directly, view it on GitHub https://github.com/ranaroussi/quantstats/issues/275#issuecomment-1636713476, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJWQW5GUANUVV2JXMG2XTJTXQJK5TANCNFSM6AAAAAA2EZ3HJY . You are receiving this because you were mentioned.Message ID: @.***>

gnzsnz commented 1 year ago

i checked the code, and cagr is never called using period parameter. so the proposed fix should not break anything.

we could leave the parameter, but to be honest i think it will only create confusion. i have submitted a pull request to @ranaroussi

glformanek commented 1 year ago

Sounds good. Thanks

On Sat, Jul 15, 2023, 2:03 PM gnzsnz @.***> wrote:

i checked the code, and cagr is never called using period parameter. so the proposed fix should not break anything.

we could leave the parameter, but to be honest i think it will only create confusion. i have submitted a pull request to @ranaroussi https://github.com/ranaroussi

— Reply to this email directly, view it on GitHub https://github.com/ranaroussi/quantstats/issues/275#issuecomment-1636858445, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJWQW5B5EEEITYNOXLR7BGLXQLSO3ANCNFSM6AAAAAA2EZ3HJY . You are receiving this because you were mentioned.Message ID: @.***>

rajnishb commented 1 year ago

Moving from 252 to 365 will also fix annual volatility and many other calculations. It's plain wrong as of today and is misleading

cammclean182 commented 1 year ago

Just started playing around with quantstats and ran into this issue when running over past work to compare the output.

Somewhat agree with everything above, but wanted to add, think we should make it 365.25 by default.