[codex] Migrate volatility data loading to canonical Alphaforge - #5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the volatility forecasting pipeline’s market data loading onto Alphaforge’s canonical adapter-backed API (DataContext.from_adapters(...) / ctx.load(...)), while keeping existing feature/target interfaces intact and updating docs/examples to reflect the py312 workflow.
Changes:
- Introduces a shared
load_market_frame(...)normalizer and updates features/targets/pipeline code to use it (adapter-first, legacyfetch_panel(...)fallback). - Adds a canonical
SimulatedGARCHAdapterand updates tests/examples to use adapter registration instead of manual legacy source wiring. - Updates repository docs and examples to emphasize the canonical
py312+python -m pytestworkflow.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| volatility_forecast/targets/squared_return.py | Switches market loading to load_market_frame and adds a forward realized variance target/helper. |
| volatility_forecast/targets/range_estimators.py | Switches range-based targets to load_market_frame; intraday target now uses normalized frame. |
| volatility_forecast/sources/simulated_garch.py | Adds SimulatedGARCHAdapter implementing the canonical adapter interface. |
| volatility_forecast/sources/init.py | Exposes simulated GARCH source + adapter in the package surface. |
| volatility_forecast/pipeline.py | Makes build_default_ctx(...) adapter-first and updates returns loading via load_market_frame. |
| volatility_forecast/market_data.py | Adds canonical load_market_frame(...) + normalization utilities for adapter and legacy paths. |
| volatility_forecast/features/return_features.py | Migrates return-based feature templates to load_market_frame. |
| volatility_forecast/features/range_features.py | Migrates range-based feature templates to load_market_frame. |
| tests/test_signature_features.py | Updates signature feature tests to register SimulatedGARCHAdapter via build_default_ctx(...). |
| tests/test_pipeline_helpers.py | Expands coverage for adapter-first build_default_ctx(...) behavior and extra adapters. |
| tests/test_pipeline_end_to_end.py | Adds an adapter-only DataContext.from_adapters(...) end-to-end dataset build test. |
| README.md | Documents canonical py312 runtime expectations and test invocation. |
| examples/volatility_forecast_2.py | Updates simulated study path to build per-run adapter-backed contexts. |
| examples/volatility_forecast_1.py | Updates simulated runs to use per-run adapter-backed contexts. |
| examples/demo_online_fit_models.py | Simplifies demo to use build_default_ctx(...) and parameterizes feature lag count. |
| docs/index.md | Adds canonical Alphaforge runtime setup + validation commands. |
| AGENTS.md | Updates repo guidance on runtime bootstrapping and test commands; clarifies examples vs research layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -21,6 +23,13 @@ class ParamSpec: # type: ignore[no-redef] | |||
| def __init__(self, *args: Any, **kwargs: Any) -> None: | |||
| pass | |||
|
|
|||
| class TargetRequest: # type: ignore[no-redef] | |||
| def __init__(self, **kwargs: Any) -> None: | |||
| self.template = kwargs.get("template") | |||
| self.params = kwargs.get("params") | |||
| self.horizon = kwargs.get("horizon") | |||
| self.name = kwargs.get("name") | |||
|
|
|||
There was a problem hiding this comment.
TargetRequest is imported (and a fallback stub is defined in the ImportError branch) but never referenced in this module. This adds dead code and can trigger unused-import linting; remove the import/stub unless it’s needed for a type annotation here.
There was a problem hiding this comment.
Removed the unused TargetRequest import and fallback stub in 6a7a2ba. It was dead code in this module.
| future_terms = [_entity_shift(squared, -step) for step in range(1, horizon + 1)] | ||
| future_panel = pd.concat(future_terms, axis=1) | ||
| future_sum = future_panel.sum(axis=1, min_count=horizon) |
There was a problem hiding this comment.
forward_realized_variance_from_log_returns builds horizon_bars shifted copies of the series and concatenates them, which is O(H) extra columns and can become memory/CPU heavy at the upper horizon (e.g., 252). Consider a groupby-based rolling/aggregation approach that computes the forward sum without materializing H intermediate columns.
| future_terms = [_entity_shift(squared, -step) for step in range(1, horizon + 1)] | |
| future_panel = pd.concat(future_terms, axis=1) | |
| future_sum = future_panel.sum(axis=1, min_count=horizon) | |
| next_squared = _entity_shift(squared, -1) | |
| if isinstance(next_squared.index, pd.MultiIndex) and next_squared.index.nlevels > 1: | |
| group_levels = list(range(next_squared.index.nlevels - 1)) | |
| future_sum = next_squared.groupby(level=group_levels, group_keys=False).apply( | |
| lambda s: s.iloc[::-1].rolling(window=horizon, min_periods=horizon).sum().iloc[::-1] | |
| ) | |
| else: | |
| future_sum = next_squared.iloc[::-1].rolling( | |
| window=horizon, | |
| min_periods=horizon, | |
| ).sum().iloc[::-1] |
There was a problem hiding this comment.
Updated in 6a7a2ba. The helper now computes the forward sum with a reverse rolling window on the next-step squared returns, grouped explicitly by entity_id, so it no longer materializes horizon_bars intermediate columns. I kept the grouping keyed to entity_id because this repo indexes panels as [ts_utc, entity_id], so grouping by all-but-last level would be incorrect here.
| ) | ||
| df = panel.df.reset_index() | ||
| df = frame.reset_index() | ||
| # detect a datetime column (supports timezone-aware dtypes) |
There was a problem hiding this comment.
Duplicate comment line: the “detect a datetime column …” note appears twice back-to-back. Please remove one to avoid confusion/noise in this hot path.
| # detect a datetime column (supports timezone-aware dtypes) |
There was a problem hiding this comment.
Removed the duplicate comment line in 6a7a2ba.
| assert ctx.adapters is not None | ||
| assert "tiingo" in ctx.adapters | ||
| adapter = ctx.adapters["tiingo"] | ||
| assert adapter._api_key == "env_tiingo_key" |
There was a problem hiding this comment.
This test asserts against adapter._api_key, which is a private implementation detail of TiingoAdapter and may break with upstream Alphaforge changes. Prefer asserting via a public attribute/property (if available) or validate behavior indirectly (e.g., adapter construction inputs / that ctx uses the adapter) without relying on private fields.
| assert adapter._api_key == "env_tiingo_key" |
There was a problem hiding this comment.
Updated in 6a7a2ba. I dropped the assertion on adapter._api_key and left the test validating adapter registration, default-source resolution, and the legacy source env wiring without depending on a private upstream attribute.
![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: 6a7a2ba2f8
ℹ️ 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 "ts_utc" != out.index.names[0] or "entity_id" != out.index.names[1]: | ||
| out.index = out.index.set_names(["ts_utc", "entity_id"]) |
There was a problem hiding this comment.
In _normalize_market_frame, the branch for already-indexed data uses set_names when level order is not ("ts_utc", "entity_id"), but set_names only renames levels and does not reorder them. If an upstream frame is indexed as (entity_id, ts_utc), this mislabels entity values as timestamps, and the later ts_utc comparisons/filters can fail or produce misaligned outputs. This makes the new normalization path break valid MultiIndex inputs that were previously acceptable.
Useful? React with 👍 / 👎.
| if end is not None: | ||
| out = out[out.index.get_level_values("ts_utc") <= _ts_utc(end)] |
There was a problem hiding this comment.
This helper sessionizes daily obs_date/date values to market-close ts_utc, then reapplies end filtering on that sessionized timestamp. For the common case where end is midnight UTC of a calendar day, the same day’s close (later in UTC) is excluded, so the final trading day is silently dropped from features/targets/returns. Since the end bound is already passed to ctx.load/fetch_panel before normalization, this post-filter introduces an off-by-one truncation in daily pipelines.
Useful? React with 👍 / 👎.
What changed
load_market_frame(...)normalizer and adapter-backed simulated data pathpy312+python -m pytestworkflowWhy
The repo had been relying on Alphaforge compatibility surfaces such as manual source wiring and
fetch_panel(...). This change makes the canonical adapter-backed API the default path while preserving the current volatility-model interfaces.Impact
build_default_ctx(...)is now adapter-firstValidation
conda run -n py312 python -m pytest -q tests/test_pipeline_helpers.py tests/test_pipeline_end_to_end.py tests/test_feature_target_alignment.py tests/test_multilag_alignment.py tests/test_forward_realized_variance_target.py tests/test_yz_and_intraday.py tests/test_signature_features.pyNotes