-
-
Notifications
You must be signed in to change notification settings - Fork 847
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
CAGR % Calculation in reports.html #275
Comments
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. |
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.
The thing is that i cant think of a case that requires periods different than 365. this piece of code |
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
<#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
<#275 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJWQW5GUANUVV2JXMG2XTJTXQJK5TANCNFSM6AAAAAA2EZ3HJY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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 |
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
<#275 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJWQW5B5EEEITYNOXLR7BGLXQLSO3ANCNFSM6AAAAAA2EZ3HJY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Moving from 252 to 365 will also fix annual volatility and many other calculations. It's plain wrong as of today and is misleading |
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. |
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
The text was updated successfully, but these errors were encountered: