-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Adjust for stock splits in portfolio engine #4195
Conversation
if not splits_df.empty : | ||
splits_df = splits_df.tz_localize(tz=None) | ||
for split_date in splits_df.index : | ||
self.__transactions['Quantity'] = np.where((self.__transactions['Ticker'] == ticker) & (self.__transactions['Date'] < split_date), self.__transactions['Quantity'] * splits_df.loc[split_date].values , self.__transactions['Quantity']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have the condition reversed. If the date is > split_date, then we multiply. For np.where it yields the first value if condition is True.
We also need to be careful and make sure that get_splits is sliced so that anything that happened before our initial purchase doesn't count )i.e, splits_df = splits_df[splits_df.index > self.__transactions['Date'][0]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are holding the scrip before the event (split) then quantity will be multiplied. I think that the condition is right. I ran with Apple as an example and worked fine. For example AAPL 1 share bought before first split date 1987-06-16 should be 224 shares now.
Slicing is also taken cared off as it iterates through splits. AAPL shares bought during 2010 should be 28 shared today. To be on the safe maybe side split_df can be sorted using index ( date).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have an example. If I bought 1 share of appl at the beginning of 2005, it underwent splits of 2,7,4, so 1 share then = 56 now. Using your code says we initially bought 56:
If I switch the < to > but dont slice split, it accounts for everything historical, saying I started at 4 shares
If I slice splits to not include before we bought, we get the desired behavior:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see where I am looking differently. Your method is correct if I enter into the template how many shares I bought from adjusted price. So if I bought 400$ of appl a few years ago, I only got 1 share. If you enter that as 4 shares at 100$ into your excel, then your split adjustment is correct. If you enter into the excel as 1 share at 400, then you need to do the reverse way I have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR I have inserted the code in the portfolio_engine.py under the preprocess_transactions.. I modify the quantity immediately after reading the transactions so that rest is taken cared of. I checked with GOOG and AAPL and it worked correctly.
This was my input
Date | Ticker | Type | Price | Quantity | Side | Currency | Fees | yf_Ticker |
---|---|---|---|---|---|---|---|---|
02/11/22 | GOOGL | Stock | 89.25 | 5 | Buy | USD | 1 | GOOGL |
27/09/22 | GOOGL | Stock | 99.75 | 5 | Buy | USD | 1 | GOOGL |
26/04/22 | GOOGL | Stock | 2400 | 1 | Buy | USD | 1 | GOOGL |
22/04/22 | GOOGL | Stock | 2400 | 1 | Buy | USD | 1 | GOOGL |
from openbb_terminal.sdk import openbb
transactions_path = '/Users/prabu/OpenBBUserData/portfolio/holdings/test_split.xlsx'
P = openbb.portfolio.load(transactions_path)
P.get_transactions()
This is the output I got..
Date | Type | Ticker | Side | Price | Quantity | Fees | Investment | Currency | Sector | Industry | Country | Region | |
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
0 | 02/11/22 | STOCK | GOOGL | Buy | 89.25 | 5 | 1 | 447.25 | USD | Communication Services | Internet Content & Information | United States | North America |
1 | 27/09/22 | STOCK | GOOGL | Buy | 99.75 | 5 | 1 | 499.75 | USD | Communication Services | Internet Content & Information | United States | North America |
2 | 26/04/22 | STOCK | GOOGL | Buy | 120 | 20 | 1 | 2401 | USD | Communication Services | Internet Content & Information | United States | North America |
3 | 22/04/22 | STOCK | GOOGL | Buy | 120 | 20 | 1 | 2401 | USD | Communication Services | Internet Content & Information | United States | North America |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see whats conflicting here. You are applying to the data frame that contains historical price data. Im applying the logic to transactions itself ( before it gets the historical price). I found it easier to adjust price and quantity in transactions itself.
splits_df = splits_df.tz_localize(tz=None) | ||
for split_date in splits_df.index : | ||
self.__transactions['Quantity'] = np.where((self.__transactions['Ticker'] == ticker) & (self.__transactions['Date'] < split_date), self.__transactions['Quantity'] * splits_df.loc[split_date].values , self.__transactions['Quantity']) | ||
self.__transactions['Price'] = np.where((self.__transactions['Ticker'] == ticker) & (self.__transactions['Date'] < split_date), self.__transactions['Price'] / splits_df.loc[split_date].values , self.__transactions['Price']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also isn't this not needed because the price is already split adjusted (which is why we need to adjust quantity in the first place)? If the value is Price * Quantity and its incorrect, then Price / Split * Quantity * Split is the same and will still be incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes Im aware that we use Adj. Close. But if we don't adjust the price here in get_transactions one will get quantity that is after split but price that is before split. Hence its introduced here.
I haven't looked into the code but basically what my expectation is, is that the volume and price is corrected after the split in which the user is informed this happend. So the following (values are random). Please note the remark regarding Yahoo Finance. If this source is used, the split is factored in within all historical data. We will also need an argument in which you can disable this correction if you entered values that are split-adjusted already (which you do by default if you would make an example portfolio with Yahoo Finance data) Alternatively, you can also adjust the historical prices of Yahoo Finance which would correctly reflect the actual historical prices. I believe this is what @jmaslek does? Why I wouldn't like it as much is because it doesn't reflect your actual current positions (given that you gained a lot more shares). |
@JerBouma for the example here
with the modified code.. I got the following as portfolio returns |
Yes, this is correct. @jmaslek Can you take it from here? This is how it should be. Note again that Yahoo Finance adjusts all prices also those before the split. |
can merge once the linters pass |
a243396
to
3473c4a
Compare
01c4287
to
590f670
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #4195 +/- ##
==========================================
Coverage ? 55.99%
==========================================
Files ? 591
Lines ? 53860
Branches ? 0
==========================================
Hits ? 30157
Misses ? 23703
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
* Adjust for stock splits in portfolio engine * Lint * Lint2 --------- Co-authored-by: James Maslek <jmaslek11@gmail.com>
@JerBouma @jmaslek
As we discussed over twitter ( https://twitter.com/jrp980/status/1623386325142347784?s=46&t=_PF5iVHF-dn4emakfIM6ag) about the stock split bug in portfolio_engine. Here is the fix to adjust for stock splits.
#4182
here is the PR for that. Its done in step 5 of __preprocess_transactions.