Track outcomes for portfolio decisions#28
Conversation
There was a problem hiding this comment.
Pull request overview
Adds local-only outcome tracking for Trading Lab daily decisions and suggested order ideas, appending decision snapshots to a gitignored CSV and later updating those rows with realized forward returns using local market CSVs. This closes #20 by enabling lightweight evaluation of whether the decision guidance helps over 1d/3d/5d/10d horizons without contacting brokers or running full workflows.
Changes:
- Introduces
trading_lab.portfolio.outcomesto record decision context intodata/processed/portfolio/decision_outcomes.csvand later update rows with forward prices/returns from local market data. - Adds CLI commands (
tl portfolio outcome-*andtl outcome *) and atl decide --record-outcomeopt-in to record outcomes during a decision run. - Extends the local portfolio GUI with forms + a “Recent outcomes” table, plus comprehensive tests and docs updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_portfolio_outcomes.py |
Adds synthetic-file-based tests covering append/update/read/format and CLI behaviors for outcomes. |
tests/test_portfolio_gui.py |
Verifies the GUI renders outcome sections and that GUI form actions create outcome rows. |
src/trading_lab/portfolio/outcomes.py |
New module implementing append/read/update/format logic for local decision outcome tracking. |
src/trading_lab/portfolio/gui_render.py |
Renders an outcomes card + recent outcomes table on the daily tab. |
src/trading_lab/portfolio/gui_forms.py |
Adds POST actions for /outcome/record and /outcome/update. |
src/trading_lab/cli/main.py |
Adds outcome CLI commands and --record-outcome support to tl decide. |
docs/local_portfolio.md |
Documents how to record, update, and view outcomes, including CLI aliases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| future = [(row_date, price) for row_date, price in prices if row_date > decision_date] | ||
| base = _float_or_none(out.get("traded_price_at_decision")) or _price_on_or_before( | ||
| prices, | ||
| decision_date, | ||
| ) | ||
| if base is not None and not out.get("current_price"): | ||
| out["current_price"] = _number(base) | ||
| if base is not None and not out.get("traded_price_at_decision"): | ||
| out["traded_price_at_decision"] = _number(base) | ||
|
|
||
| original = dict(out) | ||
| for horizon in HORIZONS: | ||
| price_key = f"future_price_{horizon}d" | ||
| return_key = f"return_{horizon}d" | ||
| if len(future) >= horizon: | ||
| future_price = future[horizon - 1][1] | ||
| out[price_key] = _number(future_price) | ||
| out[return_key] = _number(future_price / base - 1.0) if base else "" | ||
| else: | ||
| out.setdefault(price_key, "") | ||
| out.setdefault(return_key, "") | ||
|
|
||
| if all(out.get(f"future_price_{horizon}d") for horizon in HORIZONS): | ||
| out["outcome_status"] = "UPDATED" | ||
| elif any(out.get(f"future_price_{horizon}d") for horizon in HORIZONS): | ||
| out["outcome_status"] = "INSUFFICIENT_FUTURE_DATA" | ||
| else: | ||
| out["outcome_status"] = "PENDING" if future else "INSUFFICIENT_FUTURE_DATA" | ||
| return out, out != original |
| out.setdefault(price_key, "") | ||
| out.setdefault(return_key, "") |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025a99dd2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if command == "list": | ||
| from trading_lab.portfolio.outcomes import format_outcomes, read_outcomes | ||
|
|
||
| print(format_outcomes(read_outcomes(limit=args.limit))) |
There was a problem hiding this comment.
tl outcome path from missing limit
When tl outcome is run without a subcommand, command defaults to "list", but args.limit is only defined on the outcome list subparser. In that invocation, this branch raises AttributeError instead of listing outcomes, so the documented default behavior for the top-level alias is broken unless the user explicitly types list.
Useful? React with 👍 / 👎.
| if all(out.get(f"future_price_{horizon}d") for horizon in HORIZONS): | ||
| out["outcome_status"] = "UPDATED" | ||
| elif any(out.get(f"future_price_{horizon}d") for horizon in HORIZONS): | ||
| out["outcome_status"] = "INSUFFICIENT_FUTURE_DATA" |
There was a problem hiding this comment.
If a symbol has future prices but no price on/before the decision date, base is None, so returns are left blank, but the status logic still promotes the row to INSUFFICIENT_FUTURE_DATA or even UPDATED based only on future_price_* fields. That misclassifies rows whose decision-time price is still missing and can make outcome quality look better than it is.
Useful? React with 👍 / 👎.
Adds local gitignored outcome tracking for tl decisions and suggested order ideas using local market data. Closes #20.