Skip to content

Fix lp_portfolio price alignment when ticker orders differ - #767

Open
kadyrbekovhamit-cyber wants to merge 1 commit into
PyPortfolio:mainfrom
kadyrbekovhamit-cyber:fix/lp-price-label-alignment
Open

kadyrbekovhamit-cyber wants to merge 1 commit into
PyPortfolio:mainfrom
kadyrbekovhamit-cyber:fix/lp-price-label-alignment

Conversation

@kadyrbekovhamit-cyber

Copy link
Copy Markdown

lp_portfolio pairs the weights in dictionary order with latest_prices.values in Series order. When those orders differ, the returned share counts are attached to the wrong tickers, and the reported leftover can disagree with the cost of the returned allocation.

For example, with a $10,000 budget:

weights = {"BBB": 0.5, "AAA": 0.5}
prices = pd.Series({"AAA": 10.0, "BBB": 1000.0})
DiscreteAllocation(weights, prices).lp_portfolio(solver="SCIPY")

Before this fix the allocation is {'BBB': 500, 'AAA': 5} with zero leftover. Pricing that allocation by ticker costs $500,050. After the fix it is {'BBB': 5, 'AAA': 500}, which costs $10,000. This is a synthetic reproduction, not a report of executed trades or losses.

Select prices by the weight tickers before constructing the optimization problem. No objective, solver policy or API change is needed. Tests cover both Series orders and a reversed price Series from the repository's stock-price fixture, checking the independent identity sum(price[ticker] * shares) + leftover == budget.

Validation on main a6638d2e06dae6f444fd022cfd4b3c528902a85b:

  • Before the one-line fix: 2 new regression cases fail; the already aligned case passes.
  • After: python -m pytest tests/test_discrete_allocation.py -q → 20 passed.
  • git diff --check passes.

Prepared with AI assistance as part of GERO's numerical correctness work. The examples and tests were executed locally.

Signed-off-by: kadyrbekovhamit-cyber <288885044+kadyrbekovhamit-cyber@users.noreply.github.com>
@arhancanli

Copy link
Copy Markdown

Read the diff against discrete_allocation.py on main: the bug is real. lp_portfolio builds w from self.weights (weights order) but p from self.latest_prices.values (price-Series order), and the final zip([i[0] for i in self.weights], vals) assumes both match. With weights {BBB, AAA} and prices ordered AAA, BBB, the solver prices BBB at AAA's 10 and the allocation spends the wrong money, which the first new test catches through its spent <= 10000 check.

The .loc[[ticker for ticker, _ in self.weights]] fix also changes two other behaviours that I think deserve a test or a line in the description:

  • A ticker in the weights that is missing from latest_prices now raises a KeyError from .loc; before, it either silently misaligned or failed later on a shape mismatch. That is better, but a test pinning the error type would stop it changing by accident.
  • If latest_prices has extra tickers that are not in the weights, n = len(p) used to differ from len(w) and break the problem. With the fix those extra prices are ignored, which seems right, and the greedy_portfolio path already looks prices up by ticker (self.latest_prices[ticker]), so the two methods now agree.

Nothing to change in the fix itself.

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

Successfully merging this pull request may close these issues.

2 participants