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

Is get_returns correct for getting portfolio return? #104

Closed
pzzhang opened this issue Jul 29, 2023 · 4 comments
Closed

Is get_returns correct for getting portfolio return? #104

pzzhang opened this issue Jul 29, 2023 · 4 comments

Comments

@pzzhang
Copy link
Contributor

pzzhang commented Jul 29, 2023

Describe the bug
In https://github.com/ssantoshp/Empyrial/blob/main/src/empyrial/main.py#L122-L127, we compute the return of a portfolio as below.
if len(stocks) > 1:
ret_data = assets.pct_change()[1:]
returns = (ret_data * wts).sum(axis=1)

Is this correct?

Expected behavior
As the time changes, the weights of stocks in the portfolio are changing. Should wts be updated when computing the return?

@santoshlite
Copy link
Owner

Hey, what are you trying to achieve? You can use rebalancing to keep the same weights over time. Are you referring to something similar to #77? I just noticed it and will work on that!

@pzzhang
Copy link
Contributor Author

pzzhang commented Aug 5, 2023

@ssantoshp , yes, I'm referring to the same issue to #77.

I have a fix in this diff: pzzhang@1ac7290

@santoshlite
Copy link
Owner

santoshlite commented Aug 6, 2023

Oh my god, you're fantastic! It would have taken me some time to get back and remember how we designed this (I am not the one who implemented rebalancing). Would you like to make a pull request? I don't mind copying the file either, up to you.

@santoshlite
Copy link
Owner

Fixed

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

2 participants