diff --git a/alloc/core.py b/alloc/core.py index b215ef2..0777d7f 100644 --- a/alloc/core.py +++ b/alloc/core.py @@ -21,6 +21,7 @@ from alloc.lib.cache import DiskCache from alloc.lib.client import PolygonClient from alloc.models import data as data_module +from alloc.models.data import StateBuilder from alloc.models.networks import ActorCriticNetworks from alloc.models.portfolio import Portfolio, calculate_portfolio_reward @@ -211,6 +212,39 @@ def __init__( self.initial_value, ) + # ------------------------------------------------------------------ + # Internal helpers + # ------------------------------------------------------------------ + + def _build_state( + self, + multi_freq: dict[str, dict[str, list[float]]], + alloc_list: list[float], + ) -> np.ndarray: + """Build state vector using StateBuilder (TICKET-043). + + Replaces the legacy ``data_pipeline.build_state_vector`` call with + the OOP ``StateBuilder`` for consistent, testable state construction. + + Parameters + ---------- + multi_freq : dict + Multi-frequency price data from ``data_pipeline.get_multi_asset_data``. + alloc_list : list[float] + Current allocation weights per ticker. + + Returns + ------- + np.ndarray + 1-D float64 state vector. + """ + builder = StateBuilder( + hourly_window=5, + daily_window=5, + weekly_window=5, + ) + return builder.build_state(multi_freq, alloc_list) + # ------------------------------------------------------------------ # Public API # ------------------------------------------------------------------ @@ -269,14 +303,7 @@ def run(self, trading_days: int) -> dict[str, Any]: current_alloc_dict.get(t, 0.0) for t in self.tickers ] - state = self.data_pipeline.build_state_vector( - multi_freq, - alloc_list, - self.tickers, - n_hourly=5, - n_daily=5, - n_weekly=5, - ) + state = self._build_state(multi_freq, alloc_list) # Ensure state matches network input_dim if state.shape[0] != self.networks.input_dim: @@ -344,14 +371,7 @@ def run(self, trading_days: int) -> dict[str, Any]: next_alloc_list = [ next_alloc_dict.get(t, 0.0) for t in self.tickers ] - next_state = self.data_pipeline.build_state_vector( - next_multi_freq, - next_alloc_list, - self.tickers, - n_hourly=5, - n_daily=5, - n_weekly=5, - ) + next_state = self._build_state(next_multi_freq, next_alloc_list) if next_state.shape[0] != self.networks.input_dim: if next_state.shape[0] < self.networks.input_dim: next_state = np.pad( diff --git a/alloc/models/data.py b/alloc/models/data.py index 161b894..fc930a1 100644 --- a/alloc/models/data.py +++ b/alloc/models/data.py @@ -8,6 +8,8 @@ import numpy as np +from alloc.lib.cache import cache_historical, cache_latest_prices + logger = logging.getLogger(__name__) @@ -39,10 +41,11 @@ def __init__( self.weekly_window = weekly_window def _normalize_window(self, prices: list[float]) -> list[float]: - """Normalise *prices* by dividing each by the last price, then subtract 1. + """Normalise *prices* by dividing each by the last price. - The result expresses each price as a fractional change relative to - the most recent price. The last element is always ``0.0``. + The result expresses each price as a fraction of the most recent + price. The last element is always ``1.0`` (matching the legacy + ``build_state_vector`` normalisation for backward compatibility). Parameters ---------- @@ -62,7 +65,7 @@ def _normalize_window(self, prices: list[float]) -> list[float]: if last == 0.0: return [0.0] * len(prices) - return [(p / last) - 1.0 for p in prices] + return [p / last for p in prices] def _pad_window(self, prices: list[float], target_length: int) -> list[float]: """Pad *prices* with leading zeros so the result has *target_length* elements. @@ -113,9 +116,10 @@ def build_state( Returns ------- np.ndarray - 2-D float64 array of shape ``(1, N)`` where + 1-D float64 array of shape ``(N,)`` where ``N = len(tickers) * (hourly_window + daily_window + weekly_window) - + len(allocation)``. + + len(allocation)``. Matches the legacy ``build_state_vector`` + output shape for drop-in replacement in the training pipeline. """ tickers = sorted(price_data.keys()) parts: list[float] = [] @@ -146,13 +150,14 @@ def build_state( # Append allocation parts.extend(allocation) - return np.array(parts, dtype=np.float64).reshape(1, -1) + return np.array(parts, dtype=np.float64) # --------------------------------------------------------------------------- # Legacy function -- kept for backward compatibility # --------------------------------------------------------------------------- +@cache_historical() def get_multi_asset_data( tickers: list[str], client: Any, @@ -325,6 +330,7 @@ def _normalise(prices: list[float], n: int) -> list[float]: # Latest prices # --------------------------------------------------------------------------- +@cache_latest_prices() def fetch_latest_prices( tickers: list[str], client: Any, @@ -345,6 +351,8 @@ def fetch_latest_prices( """ prices: dict[str, float] = {} + cache_valid = True + for ticker in tickers: formatted = ticker.upper() try: @@ -355,9 +363,14 @@ def fetch_latest_prices( logger.debug("Latest price for %s: %.2f", formatted, price) else: prices[ticker] = 0.0 + cache_valid = False logger.warning("No valid trade for %s", formatted) except Exception as exc: logger.warning("Error fetching latest price for %s: %s", formatted, exc) prices[ticker] = 0.0 + cache_valid = False + + if not cache_valid: + prices["__cache_valid__"] = False return prices diff --git a/alloc/utils/workflow.py b/alloc/utils/workflow.py index 84df222..7b17a85 100644 --- a/alloc/utils/workflow.py +++ b/alloc/utils/workflow.py @@ -84,8 +84,9 @@ def __post_init__(self) -> None: Raises ------ ValueError - If tickers list is empty, positions dict is empty, or any - position value is zero or negative. + If tickers list is empty, positions dict is empty, any + position value is zero or negative, or there is a mismatch + between the set of tickers and the set of position keys. """ if not self.tickers: raise ValueError( @@ -104,6 +105,26 @@ def __post_init__(self) -> None: "All position values must be strictly positive." ) + # TICKET-041: Cross-validate tickers and positions + ticker_set = set(self.tickers) + position_set = set(self.positions.keys()) + + missing_in_positions = ticker_set - position_set + if missing_in_positions: + raise ValueError( + f"Tickers without corresponding positions: " + f"{sorted(missing_in_positions)}. " + "Every ticker must have a matching entry in positions." + ) + + extra_in_positions = position_set - ticker_set + if extra_in_positions: + raise ValueError( + f"Positions without corresponding tickers: " + f"{sorted(extra_in_positions)}. " + "Every position key must appear in the tickers list." + ) + @dataclass class TrainingTrial: diff --git a/tests/test_cli.py b/tests/test_cli.py index b487747..aba50d1 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1031,3 +1031,98 @@ def test_json_with_non_numeric_value_raises(self) -> None: """_json_string raises ArgumentTypeError when value is non-numeric.""" with pytest.raises(argparse.ArgumentTypeError, match="must be numeric"): _json_string('{"AAPL": "not_a_number"}') + + +# =================================================================== +# TICKET-041: Ticker-position consistency validation +# =================================================================== + + +class TestTickerPositionConsistency: + """Tests for TICKET-041: ticker-position cross-validation in TrainingConfig.""" + + def test_matching_tickers_and_positions(self) -> None: + """Matching tickers and positions should pass validation.""" + config = TrainingConfig( + tickers=["AAPL", "MSFT"], + positions={"AAPL": 50000.0, "MSFT": 50000.0}, + ) + assert config.tickers == ["AAPL", "MSFT"] + + def test_ticker_missing_from_positions(self) -> None: + """Ticker without a corresponding position should raise ValueError.""" + with pytest.raises(ValueError, match="Tickers without corresponding positions"): + TrainingConfig( + tickers=["AAPL", "MSFT", "GOOG"], + positions={"AAPL": 50000.0, "MSFT": 50000.0}, + ) + + def test_position_extra_ticker_not_in_tickers(self) -> None: + """Position key not in tickers list should raise ValueError.""" + with pytest.raises(ValueError, match="Positions without corresponding tickers"): + TrainingConfig( + tickers=["AAPL", "MSFT"], + positions={"AAPL": 50000.0, "MSFT": 50000.0, "GOOG": 30000.0}, + ) + + def test_empty_tickers_raises(self) -> None: + """Empty tickers list should raise ValueError.""" + with pytest.raises(ValueError, match="Tickers list is empty"): + TrainingConfig( + tickers=[], + positions={"AAPL": 50000.0}, + ) + + def test_empty_positions_raises(self) -> None: + """Empty positions dict should raise ValueError.""" + with pytest.raises(ValueError, match="Positions dictionary is empty"): + TrainingConfig( + tickers=["AAPL"], + positions={}, + ) + + def test_zero_position_value_raises(self) -> None: + """Zero position value should raise ValueError.""" + with pytest.raises(ValueError, match="Position value.*is 0"): + TrainingConfig( + tickers=["AAPL"], + positions={"AAPL": 0.0}, + ) + + def test_negative_position_value_raises(self) -> None: + """Negative position value should raise ValueError.""" + with pytest.raises(ValueError, match="Position value.*is -100"): + TrainingConfig( + tickers=["AAPL"], + positions={"AAPL": -100.0}, + ) + + def test_invalid_json_positions_in_build_config(self) -> None: + """build_config should raise ValueError for non-dict positions_values.""" + args = argparse.Namespace( + ticker_list=["AAPL"], + positions_values="not a dict", + iterations=1, + update_iterations=1, + trading_days=222, + batch_size=22, + min_allocation=0.001, + concentration_penalty=0.001, + transaction_cost=0.0, + risk_aversion=0.001, + min_cash_allocation=0.05, + target_sharpe=2.1, + target_value=220000.0, + target_outperformance=15.0, + ) + with pytest.raises(ValueError, match="Positions must be a JSON object"): + build_config(args) + + def test_both_mismatch_raises_first_error(self) -> None: + """When both tickers missing from positions AND extra positions exist, + the missing-from-positions error is raised first.""" + with pytest.raises(ValueError, match="Tickers without corresponding positions"): + TrainingConfig( + tickers=["AAPL", "MSFT", "GOOG"], + positions={"AAPL": 50000.0, "TSLA": 30000.0}, + ) diff --git a/tests/test_core.py b/tests/test_core.py index 582b344..468bb12 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -693,3 +693,111 @@ def test_main_calls_runner_and_saves_results(self, tmp_path: Path) -> None: # Results file should be created inside model_path assert (model_path / "backtest_results.json").exists() + + +# ===================================================================== +# TICKET-043: StateBuilder wired into training pipeline +# ===================================================================== + + +class TestStateBuilderWiring: + """Tests for TICKET-043: StateBuilder replaces build_state_vector in core.""" + + def test_simulation_runner_has_build_state_method(self) -> None: + """SimulationRunner should have _build_state method.""" + from alloc.core import SimulationRunner + assert hasattr(SimulationRunner, "_build_state") + + def test_build_state_returns_1d_array(self) -> None: + """_build_state should return a 1-D numpy array.""" + from alloc.core import SimulationRunner + from alloc.models import data as data_module + from alloc.models.networks import ActorCriticNetworks + networks = ActorCriticNetworks( + input_dim=17, + num_assets=2, + min_cash_allocation=0.05, + ) + runner = SimulationRunner( + tickers=["AAPL"], + initial_value=100000.0, + networks=networks, + data_pipeline=data_module, + client=MagicMock(), + ) + multi_freq = { + "AAPL": { + "hourly": [100.0, 101.0, 102.0, 103.0, 104.0], + "daily": [90.0, 92.0, 94.0, 96.0, 98.0], + "weekly": [80.0, 85.0, 90.0, 95.0, 100.0], + }, + } + state = runner._build_state(multi_freq, [1.0]) + assert isinstance(state, np.ndarray) + assert state.ndim == 1 + + def test_build_state_matches_legacy_normalization(self) -> None: + """_build_state normalization should match legacy build_state_vector.""" + from alloc.core import SimulationRunner + from alloc.models import data as data_module + from alloc.models.networks import ActorCriticNetworks + networks = ActorCriticNetworks( + input_dim=17, + num_assets=2, + min_cash_allocation=0.05, + ) + runner = SimulationRunner( + tickers=["AAPL"], + initial_value=100000.0, + networks=networks, + data_pipeline=data_module, + client=MagicMock(), + ) + multi_freq = { + "AAPL": { + "hourly": [100.0, 200.0, 300.0, 400.0, 500.0], + "daily": [10.0, 20.0, 30.0, 40.0, 50.0], + "weekly": [5.0, 10.0, 15.0, 20.0, 25.0], + }, + } + # StateBuilder via _build_state + sb_state = runner._build_state(multi_freq, [1.0]) + # Legacy build_state_vector + legacy_state = data_module.build_state_vector( + multi_freq, [1.0], ["AAPL"], + n_hourly=5, n_daily=5, n_weekly=5, + ) + # Should produce identical values + np.testing.assert_array_almost_equal(sb_state, legacy_state) + + def test_build_state_last_element_is_allocation(self) -> None: + """Last element of state vector should be the allocation value.""" + from alloc.core import SimulationRunner + from alloc.models import data as data_module + from alloc.models.networks import ActorCriticNetworks + networks = ActorCriticNetworks( + input_dim=17, + num_assets=2, + min_cash_allocation=0.05, + ) + runner = SimulationRunner( + tickers=["AAPL"], + initial_value=100000.0, + networks=networks, + data_pipeline=data_module, + client=MagicMock(), + ) + multi_freq = { + "AAPL": { + "hourly": [100.0, 101.0, 102.0, 103.0, 104.0], + "daily": [90.0, 92.0, 94.0, 96.0, 98.0], + "weekly": [80.0, 85.0, 90.0, 95.0, 100.0], + }, + } + state = runner._build_state(multi_freq, [0.75]) + assert abs(state[-1] - 0.75) < 1e-10 + + def test_core_imports_statebuilder(self) -> None: + """alloc.core should import StateBuilder.""" + import alloc.core + assert hasattr(alloc.core, "StateBuilder") diff --git a/tests/test_data.py b/tests/test_data.py index 9c4b4f3..7fa9876 100644 --- a/tests/test_data.py +++ b/tests/test_data.py @@ -13,6 +13,7 @@ import numpy as np import pytest +import alloc.models.data as data_module from alloc.models.data import ( build_state_vector, fetch_latest_prices, @@ -415,3 +416,86 @@ def side_effect(ticker): assert result["AAPL"] == 150.0 assert result["MSFT"] == 300.0 assert result["GOOGL"] == 140.0 + + +# ===================================================================== +# TICKET-042: Cache decorators on data pipeline +# ===================================================================== + + +class TestCacheDecoratorsApplied: + """Tests for TICKET-042: cache decorators on data pipeline functions.""" + + def test_get_multi_asset_data_has_cache_decorator(self) -> None: + """get_multi_asset_data should be decorated with @cache_historical.""" + func = data_module.get_multi_asset_data + # functools.wraps preserves __wrapped__ + assert hasattr(func, "__wrapped__"), ( + "get_multi_asset_data should be wrapped by cache decorator" + ) + + def test_fetch_latest_prices_has_cache_decorator(self) -> None: + """fetch_latest_prices should be decorated with @cache_latest_prices.""" + func = data_module.fetch_latest_prices + assert hasattr(func, "__wrapped__"), ( + "fetch_latest_prices should be wrapped by cache decorator" + ) + + def test_cache_valid_protocol_on_zero_price(self, mock_client: MagicMock) -> None: + """Raw fetch_latest_prices should set __cache_valid__=False when any price is zero.""" + raw_func = data_module.fetch_latest_prices.__wrapped__ + mock_client.get_last_trade.return_value = None + result = raw_func(tickers=["AAPL"], client=mock_client) + assert result.get("__cache_valid__") is False + + def test_cache_valid_protocol_on_api_error(self, mock_client: MagicMock) -> None: + """Raw fetch_latest_prices should set __cache_valid__=False on API error.""" + raw_func = data_module.fetch_latest_prices.__wrapped__ + mock_client.get_last_trade.side_effect = RuntimeError("network error") + result = raw_func(tickers=["AAPL"], client=mock_client) + assert result.get("__cache_valid__") is False + + def test_cache_valid_protocol_on_valid_prices(self, mock_client: MagicMock) -> None: + """Raw fetch_latest_prices should NOT set __cache_valid__ when all prices are valid.""" + raw_func = data_module.fetch_latest_prices.__wrapped__ + mock_client.get_last_trade.return_value = _make_trade(150.0) + result = raw_func(tickers=["AAPL"], client=mock_client) + assert "__cache_valid__" not in result + + def test_cache_valid_protocol_partial_failure(self, mock_client: MagicMock) -> None: + """Raw fetch_latest_prices should set __cache_valid__=False if any ticker fails.""" + raw_func = data_module.fetch_latest_prices.__wrapped__ + def side_effect(ticker): + if ticker == "BAD": + raise RuntimeError("fail") + return _make_trade(100.0) + + mock_client.get_last_trade.side_effect = side_effect + result = raw_func(tickers=["GOOD", "BAD"], client=mock_client) + assert result.get("__cache_valid__") is False + assert result["GOOD"] == 100.0 + assert result["BAD"] == 0.0 + + def test_cache_valid_protocol_missing_trade(self, mock_client: MagicMock) -> None: + """Raw fetch_latest_prices should set __cache_valid__=False when trade is missing.""" + raw_func = data_module.fetch_latest_prices.__wrapped__ + mock_client.get_last_trade.return_value = None + result = raw_func(tickers=["AAPL"], client=mock_client) + assert result.get("__cache_valid__") is False + assert result["AAPL"] == 0.0 + + def test_cache_valid_protocol_trade_without_price(self, mock_client: MagicMock) -> None: + """Raw fetch_latest_prices should set __cache_valid__=False when trade has no price.""" + raw_func = data_module.fetch_latest_prices.__wrapped__ + mock_client.get_last_trade.return_value = SimpleNamespace() + result = raw_func(tickers=["AAPL"], client=mock_client) + assert result.get("__cache_valid__") is False + assert result["AAPL"] == 0.0 + + def test_decorated_fetch_strips_cache_valid(self, mock_client: MagicMock) -> None: + """Decorated fetch_latest_prices should strip __cache_valid__ from result.""" + mock_client.get_last_trade.return_value = None + # Call the decorated version — __cache_valid__ should be stripped + result = fetch_latest_prices(tickers=["AAPL"], client=mock_client) + assert "__cache_valid__" not in result + assert result["AAPL"] == 0.0 diff --git a/tests/test_state_builder.py b/tests/test_state_builder.py index 0df4c0e..edb1560 100644 --- a/tests/test_state_builder.py +++ b/tests/test_state_builder.py @@ -86,21 +86,21 @@ class TestNormalizeWindow: def test_basic_normalization(self, builder_small: StateBuilder) -> None: prices = [100.0, 110.0, 120.0] result = builder_small._normalize_window(prices) - # (100/120)-1 = -0.1666..., (110/120)-1 = -0.0833..., (120/120)-1 = 0.0 + # (100/120) = 0.8333..., (110/120) = 0.9166..., (120/120) = 1.0 assert len(result) == 3 - assert abs(result[0] - (-1 / 6)) < 1e-10 - assert abs(result[1] - (-1 / 12)) < 1e-10 - assert abs(result[2] - 0.0) < 1e-10 + assert abs(result[0] - (100.0 / 120.0)) < 1e-10 + assert abs(result[1] - (110.0 / 120.0)) < 1e-10 + assert abs(result[2] - 1.0) < 1e-10 - def test_last_element_is_zero(self, builder_small: StateBuilder) -> None: + def test_last_element_is_one(self, builder_small: StateBuilder) -> None: prices = [50.0, 60.0, 70.0, 80.0] result = builder_small._normalize_window(prices) - assert abs(result[-1] - 0.0) < 1e-10 + assert abs(result[-1] - 1.0) < 1e-10 def test_single_price(self, builder_small: StateBuilder) -> None: prices = [100.0] result = builder_small._normalize_window(prices) - assert result == [0.0] + assert result == [1.0] def test_empty_prices(self, builder_small: StateBuilder) -> None: result = builder_small._normalize_window([]) @@ -124,9 +124,9 @@ def test_preserves_length(self, builder_small: StateBuilder) -> None: def test_negative_returns(self, builder_small: StateBuilder) -> None: prices = [200.0, 150.0, 100.0] result = builder_small._normalize_window(prices) - assert result[0] == 1.0 # (200/100) - 1 - assert result[1] == 0.5 # (150/100) - 1 - assert abs(result[2] - 0.0) < 1e-10 + assert result[0] == 2.0 # 200/100 + assert result[1] == 1.5 # 150/100 + assert abs(result[2] - 1.0) < 1e-10 def test_no_nan_or_inf(self, builder_small: StateBuilder) -> None: prices = [1.0, 2.0, 3.0, 4.0, 5.0] @@ -198,11 +198,12 @@ def test_returns_numpy_array( state = builder_small.build_state(sample_price_data, [0.5, 0.5]) assert isinstance(state, np.ndarray) - def test_shape_is_1d_batch( + def test_shape_is_1d( self, builder_small: StateBuilder, sample_price_data: dict ) -> None: + """StateBuilder returns 1-D array matching legacy build_state_vector.""" state = builder_small.build_state(sample_price_data, [0.5, 0.5]) - assert state.shape[0] == 1 + assert state.ndim == 1 def test_shape_width_correct( self, builder_small: StateBuilder, sample_price_data: dict @@ -210,7 +211,7 @@ def test_shape_width_correct( # 2 tickers * (3 hourly + 5 daily + 2 weekly) + 2 allocation = 20 expected_n = 2 * (3 + 5 + 2) + 2 state = builder_small.build_state(sample_price_data, [0.5, 0.5]) - assert state.shape == (1, expected_n) + assert state.shape == (expected_n,) def test_single_ticker(self, builder_small: StateBuilder) -> None: data = { @@ -222,7 +223,7 @@ def test_single_ticker(self, builder_small: StateBuilder) -> None: } state = builder_small.build_state(data, [1.0]) expected_n = 1 * (3 + 5 + 2) + 1 - assert state.shape == (1, expected_n) + assert state.shape == (expected_n,) def test_allocation_appended(self, builder_small: StateBuilder) -> None: data = { @@ -233,14 +234,14 @@ def test_allocation_appended(self, builder_small: StateBuilder) -> None: }, } state = builder_small.build_state(data, [1.0]) - assert state[0, -1] == 1.0 + assert state[-1] == 1.0 def test_two_ticker_allocation( self, builder_small: StateBuilder, sample_price_data: dict ) -> None: state = builder_small.build_state(sample_price_data, [0.3, 0.7]) - assert state[0, -2] == 0.3 - assert state[0, -1] == 0.7 + assert state[-2] == 0.3 + assert state[-1] == 0.7 def test_normalization_in_state(self, builder_small: StateBuilder) -> None: data = { @@ -251,10 +252,10 @@ def test_normalization_in_state(self, builder_small: StateBuilder) -> None: }, } state = builder_small.build_state(data, [1.0]) - # Hourly: (100/300)-1=-0.6667, (200/300)-1=-0.3333, (300/300)-1=0.0 - assert abs(state[0, 0] - (-2 / 3)) < 1e-10 - assert abs(state[0, 1] - (-1 / 3)) < 1e-10 - assert abs(state[0, 2] - 0.0) < 1e-10 + # Hourly: 100/300=0.3333, 200/300=0.6667, 300/300=1.0 + assert abs(state[0] - (100.0 / 300.0)) < 1e-10 + assert abs(state[1] - (200.0 / 300.0)) < 1e-10 + assert abs(state[2] - 1.0) < 1e-10 def test_padding_in_state(self, builder_small: StateBuilder) -> None: data = { @@ -265,10 +266,10 @@ def test_padding_in_state(self, builder_small: StateBuilder) -> None: }, } state = builder_small.build_state(data, [1.0]) - # Hourly: pad 2 zeros + normalize [100] -> [0.0, 0.0, 0.0] - assert state[0, 0] == 0.0 - assert state[0, 1] == 0.0 - assert state[0, 2] == 0.0 # (100/100)-1 = 0.0 + # Hourly: pad 2 zeros + normalize [100] -> [0.0, 0.0, 1.0] + assert state[0] == 0.0 + assert state[1] == 0.0 + assert abs(state[2] - 1.0) < 1e-10 # 100/100 = 1.0 def test_empty_data(self, builder_small: StateBuilder) -> None: data = { @@ -280,10 +281,10 @@ def test_empty_data(self, builder_small: StateBuilder) -> None: } state = builder_small.build_state(data, [1.0]) expected_n = 1 * (3 + 5 + 2) + 1 - assert state.shape == (1, expected_n) + assert state.shape == (expected_n,) # All zeros except allocation - assert state[0, -1] == 1.0 - assert np.all(state[0, :-1] == 0.0) + assert state[-1] == 1.0 + assert np.all(state[:-1] == 0.0) def test_zero_prices_no_crash(self, builder_small: StateBuilder) -> None: data = { @@ -319,8 +320,8 @@ def test_ticker_order_sorted(self, builder_small: StateBuilder) -> None: } state = builder_small.build_state(data, [0.5, 0.5]) # AAPL comes first (alphabetical), its hourly normalized: - # (100/102)-1, (101/102)-1, (102/102)-1 - assert abs(state[0, 0] - (-2 / 102)) < 1e-10 + # 100/102, 101/102, 102/102 + assert abs(state[0] - (100.0 / 102.0)) < 1e-10 def test_default_builder_large_windows( self, builder: StateBuilder @@ -335,7 +336,7 @@ def test_default_builder_large_windows( } state = builder.build_state(data, [1.0]) expected_n = 1 * (168 + 365 + 52) + 1 - assert state.shape == (1, expected_n) + assert state.shape == (expected_n,) def test_three_tickers(self, builder_small: StateBuilder) -> None: data = { @@ -357,8 +358,8 @@ def test_three_tickers(self, builder_small: StateBuilder) -> None: } state = builder_small.build_state(data, [0.33, 0.33, 0.34]) expected_n = 3 * (3 + 5 + 2) + 3 - assert state.shape == (1, expected_n) + assert state.shape == (expected_n,) # Last 3 elements are allocation - assert abs(state[0, -3] - 0.33) < 1e-10 - assert abs(state[0, -2] - 0.33) < 1e-10 - assert abs(state[0, -1] - 0.34) < 1e-10 + assert abs(state[-3] - 0.33) < 1e-10 + assert abs(state[-2] - 0.33) < 1e-10 + assert abs(state[-1] - 0.34) < 1e-10 diff --git a/tickets/TICKET-041.md b/tickets/TICKET-041.md new file mode 100644 index 0000000..6922403 --- /dev/null +++ b/tickets/TICKET-041.md @@ -0,0 +1,14 @@ +# TICKET-041: Validate ticker-position consistency in CLI and TrainingConfig + +**Module:** `alloc/cli.py`, `alloc/utils/workflow.py` +**Priority:** Medium — silent data mismatch risk + +## Evidence + +In `alloc/cli.py`, `build_config()` (line ~200) maps `args.ticker_list` and `args.positions` independently into `TrainingConfig`. There is no cross-validation that: +1. Every ticker in `--tickers` has a corresponding entry in `--positions-values` +2. Every key in `--positions-values` corresponds to a ticker in `--tickers` + +In `alloc/utils/workflow.py`, `TrainingConfig.__post_init__` (line ~70) validates that tickers is non-empty and positions is non-empty with positive values, but does **not** validate that `set(tickers) == set(positions.keys())`. + +Example of silent mismatch: diff --git a/tickets/TICKET-042.md b/tickets/TICKET-042.md new file mode 100644 index 0000000..aa62903 --- /dev/null +++ b/tickets/TICKET-042.md @@ -0,0 +1,29 @@ +# TICKET-042: Apply cache decorators to data pipeline functions per TICKET-007 spec + +**Module:** `alloc/models/data.py` +**Priority:** Medium — missing disk caching for market data fetches + +## Evidence + +TICKET-007 specification requires: +- `get_multi_asset_data()` should be decorated with `@cache_historical` from `alloc.lib.cache` +- `fetch_latest_prices()` should be decorated with `@cache_latest_prices` from `alloc.lib.cache` +- `fetch_latest_prices()` should implement the `__cache_valid__` protocol: if any ticker returns an invalid/zero price, set `result["__cache_valid__"] = False` + +Current implementation in `alloc/models/data.py`: +- `get_multi_asset_data()` (line ~140) has **no cache decorator** — every call hits the Polygon API +- `fetch_latest_prices()` (line ~290) has **no cache decorator** — every call hits the API +- Neither function imports from `alloc.lib.cache` +- `fetch_latest_prices()` does not implement `__cache_valid__` protocol + +The cache decorators exist and are functional (`alloc/lib/cache.py` lines ~130-170), with `cache_historical` and `cache_latest_prices` exported as convenience decorators. + +## Impact + +- **Excessive API calls**: Without caching, every workflow run fetches all historical data from Polygon.io, incurring rate limit pressure and slower execution. +- **No cache invalidation on bad data**: Without `__cache_valid__`, stale/zero prices could be cached and served on subsequent runs. +- **Specification non-compliance**: TICKET-007 explicitly requires these decorators; their absence means the ticket is not fully complete. + +## Suggestion + +1. Add imports at top of `alloc/models/data.py`: diff --git a/tickets/TICKET-043.md b/tickets/TICKET-043.md new file mode 100644 index 0000000..a65e208 --- /dev/null +++ b/tickets/TICKET-043.md @@ -0,0 +1,39 @@ +# TICKET-043: Wire StateBuilder into training pipeline (replace legacy build_state_vector) + +**Module:** `alloc/core.py`, `alloc/models/data.py` +**Priority:** Medium — architectural inconsistency + +## Evidence + +`alloc/models/data.py` defines two parallel implementations: + +1. **`StateBuilder` class** (line ~17) — OOP design with configurable windows, `_normalize_window`, `_pad_window`, and `build_state()` method. Returns 2-D array of shape `(1, N)`. + +2. **`build_state_vector()` function** (line ~250) — legacy standalone function. Returns 1-D array of shape `(N,)`. + +The training pipeline in `alloc/core.py` uses **only the legacy function**: +- Line 667: `data_pipeline=data_module` — passes the entire module as the pipeline +- Line 272: `self.data_pipeline.build_state_vector(...)` — calls the legacy function +- Line 347: `self.data_pipeline.build_state_vector(...)` — calls the legacy function again + +`StateBuilder` is **never instantiated or used** in `core.py`, `workflow.py`, or `cli.py`. It exists only in tests (`tests/test_state_builder.py`). + +Additionally, the two implementations differ in behavior: +- `StateBuilder._normalize_window` subtracts 1.0 after dividing by last price → last element is `0.0` +- `build_state_vector()._normalise` divides by last price without subtracting → last element is `1.0` +- `StateBuilder.build_state` returns shape `(1, N)` (2-D) +- `build_state_vector` returns shape `(N,)` (1-D) + +## Impact + +- **Dead code**: `StateBuilder` is untested in production paths — bugs in it would go undetected. +- **Inconsistent normalization**: If someone switches to `StateBuilder`, the RL agent receives different input distributions (0-centered vs 1-centered), breaking trained models. +- **Shape mismatch**: `StateBuilder` returns `(1, N)` but the pipeline expects `(N,)`. The padding logic at line 281-288 of `core.py` masks this for the 1-D case but would behave differently with 2-D input. +- **Maintenance burden**: Two implementations of the same concept means fixes must be applied twice. + +## Suggestion + +**Option A (preferred): Retire legacy function, adopt StateBuilder** + +1. Update `StateBuilder._normalize_window` to match the legacy normalization (divide by last, no subtract-1), OR update `build_state_vector` to match `StateBuilder` — pick one canonical behavior and document it. +2. Change `core.py` to instantiate `StateBuilder` instead of passing `data_module`: