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.

Sign up for GitHub

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

Jump to bottom

Update stats.py - fix cagr calculation #281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update stats.py - fix cagr calculation #281

wants to merge 1 commit into from
+2 −2

Conversation

gnzsnz
Copy link

@gnzsnz gnzsnz commented Jul 15, 2023

remove periods=252 parameter. cagr calculation done by dividing number of days by 365.

'years = (returns.index[-1] - returns.index[0]).days / periods' there is no rational for dividing number of days by anything other than 365.

fix 273 and 275

@gnzsnz
Update stats.py - fix cagr calculation
cf61c8b 
remove periods=252 paramter. cagr parameter done by dividing number of days by 365.
@tschm
Copy link

tschm commented Jul 17, 2023

The assumption is that the index only iterates over business days for which there are around 252 in year (by convention)

@silvercrw1994
Copy link

The assumption is that the index only iterates over business days for which there are around 252 in year (by convention)
years = (returns.index[-1] - returns.index[0]).days/252

if we use this assumption then period between 01/07/2005 -- 22/05/2023 will be approx 26 years. Hence this is essentially overestimating the years.

@gnzsnz
Copy link
Author

gnzsnz commented Jul 17, 2023

Hence this is essentially overestimating the years.

yes, this is exactly what is happening here.

num_days = returns.index[-1] - returns.index[0]).days the cagr function calculates the number of days like this. then does num_days/period to get years the problem is that using 252 gives the wrong number of years between 2 given dates. Using 365 fixes it.

the pull request is actually proposing to remove the periods parameter completely. the reason behind is that any value other than 365 will give the wrong number of years. And the wrong number of years impacts the CAGR. As the "A" in CARG stands for annual, then there is no need to have this as a parameter. Or at least, I can't think of a case where anything other than 365 would work. Unless the way of calculating num_days changes

@silvercrw1994
Copy link

Unless the way of calculating num_days changes

Yes
".days" attribute does not consider trading days but calculates the actual number of days between two dates.

@tiloye
Copy link

tiloye commented Jul 20, 2023

@gnzsnz, the period parameter shouldn't be removed because it will allow users to evaluate portfolios based on the time frame that's being observed. The value that is needed for the actual calculation is years = len(returns)/periods or
res = abs(total + 1) ** (periods/len(returns)) - 1.
The idea is based on the definition of CAGR discussed in the article linked below. This will allow users to calculate CAGR for any time period.

An alternative approach is to use log returns, but it's best to stick with normal returns because quantstats uses normal returns for most of its calculations. I compared log returns and normal CAGR approach on daily period, and I got approximately the same result. Using the CAGR function currently implemented in quantstats gives a different result.

If you could review your code and implement to version of CAGR discussed in the article, that would be great.

https://www.investopedia.com/terms/a/annualized-total-return.asp#:~:text=The%20main%20difference%20between%20them,two%20measures%20are%20the%20same.

@gnzsnz
Copy link
Author

gnzsnz commented Jul 21, 2023

@tiloye thanks for your feedback. you really got me thinking. i did quite some reading to understand this better, but i have to say that I still think that we should remove the periods parameter. I think is the best alternative, and I've just realized that I got it right in the first place just by chance.

The thing is that Annualized Total Return and the Compound Annual Growth Rate (CAGR) are essentially the same. Now what really got me thinking was your point stating that we should be able to evaluate CAGR for any time period. And I realized that the periods parameter is not going to achieve that.

Basically we need to calculate the number of years for the returns time series (returns parameter) and we could do that in two different ways, one would be to use the current logic and get the actual number of days between start and end date. Another option is what you propose to get the length of the time series, or number of periods.

If we go for actual number of days as it's today, we need to use 365 as the denominator. if we use length we need use 252. But that's it it could be 365 or 252, and it should be aligned with the method used to calculate the number of days.

But in any case having a parameter for periods (which is basically our denominator) can only cause problems. Because anything other than the default will return a wrong number.

if we want to have the option to calculate returns over different periods, then we need to provide different periods in the returns series, not change the periods parameter.

Hope my point is clear. As I said before, I did not realize this until I really went into it.

@tiloye
Copy link

tiloye commented Jul 21, 2023

@gnzsnz, I get your point. If we are going to use the difference between the dates, then 365 should be the denominator. I also confirmed it holds for return series that isn't up to a year. Thanks for pointing that out.

@gnzsnz
Copy link
Author

gnzsnz commented Aug 6, 2023

@ranaroussi are you OK with this PR?

@Newtoniano
Copy link

This looks good to me, does this also take care of some other possible metrics miscalculations introduced with the period parameter changes? If everything else looks properly calculated with this PR, please merge asap

@gnzsnz
Copy link
Author

gnzsnz commented Sep 2, 2023

@ranaroussi kind reminder

@gnzsnz gnzsnz mentioned this pull request Sep 13, 2023
Open
@gnzsnz gnzsnz mentioned this pull request Oct 15, 2023
Open
@gnzsnz gnzsnz mentioned this pull request Dec 14, 2023
Open
@jmy12k3
Copy link

jmy12k3 commented Jan 27, 2024

is this project abandoned? tons of unsolved PRs

@gnzsnz
Copy link
Author

gnzsnz commented Jan 27, 2024

#323 is still open

@grzesir
Copy link

grzesir commented Jan 31, 2024 •
edited

Since this library no longer seems to be maintained, we have created a duplicate of it that we plan to maintain and improve over time. If you'd like to use our version check it out here: https://github.com/Lumiwealth/quantstats_lumi

Please feel free to fork and send your pull request to this new library, we will be actively monitoring the library and approving PRs

@jmy12k3
Copy link

jmy12k3 commented Feb 1, 2024

Since this library no longer seems to be maintained, we have created a duplicate of it that we plan to maintain and improve over time. If you'd like to use our version check it out here: https://github.com/Lumiwealth/quanstats_lumi

Please feel free to fork and send your pull request to this new library, we will be actively monitoring the library and approving PRs

Cool library, Nice! I will surely take a look at it.

@grzesir
Copy link

grzesir commented Feb 1, 2024 via email

@gnzsnz
Copy link
Author

gnzsnz commented Feb 3, 2024 via email

@grzesir
Copy link

grzesir commented Feb 4, 2024 via email

@matt-the-catt
Copy link

matt-the-catt commented Apr 9, 2024 •
edited

I don't have a good solution to this problem, but just wanted to note that setting periods=365 will only make results more accurate, but will not fix the problem entirely.

This calculation:
years = (returns.index[-1] - returns.index[0]).days / periods
wrongly assumes that the first and the last day is a trading day.

If these are not trading days, doing returns.index[-1] - returns.index[0] will always result in a number less than 365 and the whole cagr calculation will be a slight overestimation.

For example, lets choose a period 2020-01-01 to 2020-12-31.
2020-01-01 was a non trading day, so when _yf downloads returns, the Series will start at 2020-01-02.
Doing (returns.index[-1] - returns.index[0]).days will be equal to 364

@grzesir
Copy link

grzesir commented Apr 9, 2024 via email

@gnzsnz
Copy link
Author

gnzsnz commented Apr 22, 2024 •
edited

Since this library no longer seems to be maintained, we have created a duplicate of it that we plan to maintain and improve over time. If you'd like to use our version check it out here: https://github.com/Lumiwealth/quantstats_lumi

Please feel free to fork and send your pull request to this new library, we will be actively monitoring the library and approving PRs

@grzesir have you considered PEP 541, you can keep pypi package name if you apply for it. I understand that you need to create an issue under pypi support and tag it as PEP 541, here is an example pypi/support#3932

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CAGR Doesn't Match Closely with Portfolio Visualizer
8 participants
@gnzsnz @tschm @silvercrw1994 @tiloye @Newtoniano @jmy12k3 @grzesir @matt-the-catt