Bugs in Portfolio.adjust_percents

[assumptionsoup]

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:

Update: I was wrong about this. The total funds are recalculated, but they shouldn't change.

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.

[fja05680]

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 https://github.com/fja05680/pinkfish/issues/70, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACD3KHSYHGQS42IS46EOZNDXONT4RANCNFSM6AAAAAAZ5BMHCE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[assumptionsoup]

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).

[fja05680]

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." adjust_percent() can be called directly by the user, so it needs to calculate total_funds. Since adjust_percents() calls adjust_percent(), we get this recalculation effect. While perhaps not optimal, it's acceptable. Update I probably misunderstood. You are saying that total_funds should be calculated once, then do all the adjustments. This makes sense, but is it more correct. Is this the way real trading would be done in an account?

(4) "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."

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

[fja05680]

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

[assumptionsoup]

Sorry for disappearing there. Life happened and such. I'm glad I could help! Likewise, thanks for writing Pinkfish!

[fja05680]

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 https://github.com/fja05680/pinkfish/issues/70#issuecomment-1661358954, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACD3KHWCSAIMRHFNAH46A2TXTGWKBANCNFSM6AAAAAAZ5BMHCE . You are receiving this because you modified the open/close state.Message ID: @.***>