-
Notifications
You must be signed in to change notification settings - Fork 57
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
Bugs in Portfolio.adjust_percents #70
Comments
Hi Jordan,
Thank you for your careful analysis of pinkfish code. I'm actually kind of
glad someone is finding an issue within the pinkfish code base so I have a
reason to work on it again. The project is definitely not abandoned. It
just does everything I need it to do right now so I don't change the code
often. Just update it for Pandas changes mostly and new examples. Anyway,
please give me a couple of days to look over your observation and I'll have
a response shortly.
Farrell
…On Mon, Jul 3, 2023 at 9:05 PM Jordan Hueckstaedt ***@***.***> wrote:
I think there are issues/bugs inside Portfolio.adjust_percents. This
could have been a pull request, but I'd rather have a discussion in case
I'm mistaken. I'll walk through what I think are the problems.
def adjust_percents(self, date, prices, weights, row, directions=None):
w = {}
# Get current weights
for symbol in self.symbols:
w[symbol] = self.share_percent(row, symbol)
Weights in other functions are converted between whole numbers and floats.
We don't know if the user is passing in whole numbers or floats yet. Later
sorting will fail if there's a mismatch. I think this should be something
like:
convert_weight = lambda weight: self._float(weight) if weight <= 1 else self._float(weight) / self._float(100)
weights = {symbol: convert_weight(weight) for symbol, weight in weights.items()}
w = {}
# Get current weights
for symbol in self.symbols:
w[symbol] = convert_weight(self.share_percent(row, symbol, field=field))
Next,
# If direction is None, this set all to pf.Direction.LONG
if directions is None:
directions = {symbol:trade.Direction.LONG for symbol in self.symbols}
# Reverse sort by weights. We want current positions first so that
# if they need to reduced or closed out, then cash is freed for
# other symbols.
w = utility.sort_dict(w, reverse=True)
This says we want to sell current positions first to obtain cash. I agree
that we need to sell current positions first, but this ordering doesn't
make sense to me. This sorts the current largest positions first, but
there's no guarantee that the largest weighted item will be a sell or a
buy. Instead, I think you need to sort the change of the current weight of
the position to the new weight of the position and order negative / sell
orders first.
Next, more issues.
# Update weights with new values.
w.update(weights)
# Call adjust_percents() for each symbol.
for symbol, weight in w.items():
price = prices[symbol]
direction = directions[symbol]
self.adjust_percent(date, price, weight, symbol, row, direction)
return w
self.adjust_percent adjusts the weight by the total cash each iteration.
But the total cash is changing each iteration. Each time this function is
run the total cash is changing. In order to set the overall weights of the
portfolio, the total cash available has to be calculated first. More like
this:
total_funds = self._total_funds(row)
def adjust_percent(date, price, weight, symbol, row, direction):
value = total_funds * weight
shares = self._adjust_value(date, price, value, symbol, row, direction)
return shares
# Call adjust_percents() for each symbol.
# Sell first to free capital
buys = {}
for symbol in set(w.keys()).union(weights.keys()):
weight = w.get(symbol, weights[symbol])
new_weight = weights.get(symbol, w[symbol])
if new_weight < weight:
adjust_percent(date, prices[symbol], new_weight, symbol, row, directions[symbol])
else:
buys[symbol] = new_weight
# Now buy
for symbol, weight in buys.items():
adjust_percent(date, prices[symbol], weight, symbol, row, directions[symbol])
w.update(weights)
return w
Finally, there's a subtler issue still lurking.
Under the hood, total_funds = self._total_funds(row) uses the close price
to determine total funds. This impacts the value going into
self._adjust_value (total_funds (at close) * weight). But the shares are
then purchased / sold at the price passed in. I think this means that if
open prices were passed in the shares would sell at close price, but then
be bought at the open price. I suspect that prices shouldn't be passed in
at all. The field should be passed and that should be used to rebalance the
portfolio. Was there some other intent? I understand we might want to
simulate slippage, but I'm not sure that adjust_percent is the right place?
I kind of liked that this framework was rather straight forward and the
intricacies of actual trading were mostly abstracted away so I could focus
more on strategies -- not whether my sells and buys were ordered perfectly,
or that technically I can't use 100% of my funds to rebalance my portfolio
perfectly.
—
Reply to this email directly, view it on GitHub
<#70>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACD3KHSYHGQS42IS46EOZNDXONT4RANCNFSM6AAAAAAZ5BMHCE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Thanks for taking a look! I didn't think pinkfish was abandoned at all -- I've been looking at the other frameworks and there are quite a few that are dead. The documentation is really great, and I think the design fits this nice space of easier-to-use than vectorbt and less fuss in the way than backtester.py (which looks more useful to me as a trading validator than as a strategy tester). |
Jordan, (1) "Weights in other functions are converted between whole numbers and floats. We don't know if the user is passing in whole numbers or floats yet. Later sorting will fail if there's a mismatch. I think this should be something like:" Looking at the code, I think the simplest thing to do would be to have the percents ALWAYS be floats between 0 and 1. I do see the inconsistency you pointed out. I don't think it would be a big burden for the user. Since my user base is still small, I can make some breaking changes to the API. (2) "This says we want to sell current positions first to obtain cash. I agree that we need to sell current positions first, but this ordering doesn't make sense to me. This sorts the current largest positions first, but there's no guarantee that the largest weighted item will be a sell or a buy. Instead, I think you need to sort the change of the current weight of the position to the new weight of the position and order negative / sell orders first." Good catch. Yea ordering doesn't make sense. Thanks for this suggestion. Let me think on it a bit. (3) " was wrong about this. The total funds are recalculated, but they shouldn't change." (4) "Finally, there's a subtler issue still lurking. pinkfish didn't originally have the portfolio module. It was single stock/etf platform for a long time. With the single trading symbol, I wanted to be able to do things like buy on the open then sell on the close on the same day. I tried to maintain that same capability for portfolio, by allowing the passing of price data other than just 'close'. As I went along, complications arose, and for me, doing portfolio trading on the 'close' is good enough to deal with all the practical cases I was interested in. You bring some good points though and I'll see if there is a better way. Ideally, any price data could be used, not just close. Regarding slippage, I don't bother with it. I just know I can't expect world real trading to be exactly the same. (1) and (2) aren't too difficult. (3) Should it be changed?. (4) Will require some thought and time. Farrell |
Jordan, I have addressed all of the issues you raised. You had some great insight into problems with adjust_percent(s) functions, especially in regard to point (4). I took nearly all of your recommendations, in particular not passing in the price values, but instead passing in the field. This not only corrected the code, but made the user interface much cleaner. Thanks you again. With that, I'm closing this issue. Farrell |
Sorry for disappearing there. Life happened and such. I'm glad I could help! Likewise, thanks for writing Pinkfish! |
Jordan, I completely understand. Same reason it took a while to get these
fixes in. Thanks for your kind words.
…On Tue, Aug 1, 2023, 9:45 PM Jordan Hueckstaedt ***@***.***> wrote:
Sorry for disappearing there. Life happened and such. I'm glad I could
help! Likewise, thanks for writing Pinkfish!
—
Reply to this email directly, view it on GitHub
<#70 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACD3KHWCSAIMRHFNAH46A2TXTGWKBANCNFSM6AAAAAAZ5BMHCE>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
I think there are issues/bugs inside
Portfolio.adjust_percents
. This could have been a pull request, but I'd rather have a discussion in case I'm mistaken. I'll walk through what I think are the problems.Weights in other functions are converted between whole numbers and floats. We don't know if the user is passing in whole numbers or floats yet. Later sorting will fail if there's a mismatch. I think this should be something like:
Next,
This says we want to sell current positions first to obtain cash. I agree that we need to sell current positions first, but this ordering doesn't make sense to me. This sorts the current largest positions first, but there's no guarantee that the largest weighted item will be a sell or a buy. Instead, I think you need to sort the change of the current weight of the position to the new weight of the position and order negative / sell orders first.
Next, more issues.
`self.adjust_percent` adjusts the weight by the total cash each iteration. But the total cash is changing each iteration. Each time this function is run the total cash is changing. In order to set the overall weights of the portfolio, the total cash available has to be calculated first. More like this:Update: I was wrong about this. The total funds are recalculated, but they shouldn't change.
Finally, there's a subtler issue still lurking.
Under the hood,
total_funds = self._total_funds(row)
uses the close price to determine total funds. This impacts the value going intoself._adjust_value
(total_funds (at close) * weight). But the shares are then purchased / sold at the price passed in. I think this means that if open prices were passed in the shares would sell at close price, but then be bought at the open price. I suspect that prices shouldn't be passed in at all. The field should be passed and that should be used to rebalance the portfolio. Was there some other intent? I understand we might want to simulate slippage, but I'm not sure that adjust_percent is the right place? I kind of liked that this framework was rather straight forward and the intricacies of actual trading were mostly abstracted away so I could focus more on strategies -- not whether my sells and buys were ordered perfectly, or that technically I can't use 100% of my funds to rebalance my portfolio perfectly.The text was updated successfully, but these errors were encountered: