Add offline calibrate CSV CLI#53
Conversation
Constraint: Issue YichengYang-Ethan#33 asks for a thin CLI over the existing WangMLE estimator. Rejected: Add pandas as a new direct dependency | the repository does not declare pandas directly, and csv.DictReader is sufficient for the required columns. Confidence: medium Scope-risk: narrow Directive: Keep calibration CLI as an estimator wrapper; do not add trading or financial-advice behavior here. Tested: uv run --python 3.12 --with click --with rich --with numpy --with scipy --with pytest --with pytest-cov --with pytest-asyncio --with pydantic --with requests --with beautifulsoup4 --with feedparser --with httpx --with openai --with openai-agents --with py-clob-client --with tenacity --with textual --with cryptography --with solana --with solders --with kalshi-python python -m pytest tests\test_calibrate_cli.py -q Tested: uv run --python 3.12 --with ruff ruff check oracle3\cli\cli.py tests\test_calibrate_cli.py Tested: git diff --check Not-tested: Full repository pytest/mypy; local proof used a targeted dependency set for the new CLI path.
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'calibrate' CLI command to estimate the Wang Transform lambda from a resolved-contract CSV, along with supporting helper functions and test coverage. The review feedback highlights three key improvements: parsing CSV outcomes as floats to robustly handle float-formatted strings (e.g., from Pandas), avoiding encapsulation violations by determining covariate counts from the design matrix shape instead of accessing a private attribute, and expanding test coverage to include the '--hierarchical' option and its error-handling paths.
| try: | ||
| price = float(row['price']) | ||
| outcome = int(row['outcome']) | ||
| except (TypeError, ValueError) as exc: | ||
| raise click.ClickException( | ||
| f'Invalid price/outcome at CSV row {row_number}' | ||
| ) from exc | ||
|
|
||
| if not 0 < price < 1: | ||
| raise click.ClickException( | ||
| f'Invalid price at CSV row {row_number}: expected value in (0, 1)' | ||
| ) | ||
| if outcome not in (0, 1): | ||
| raise click.ClickException( | ||
| f'Invalid outcome at CSV row {row_number}: expected 0 or 1' | ||
| ) | ||
|
|
||
| parsed = {'price': price, 'outcome': float(outcome)} |
There was a problem hiding this comment.
The current implementation parses outcome as an integer using int(row['outcome']). However, if the CSV is generated by tools like Pandas or Excel, integer columns are often exported as floats (e.g., 1.0 or 0.0). In Python, int('1.0') raises a ValueError, which would cause the CLI to fail on valid data.
Additionally, outcome is converted to a float later anyway when constructing the parsed dictionary ('outcome': float(outcome)).
We can make this parsing more robust and cleaner by parsing outcome directly as a float, checking if it is 0.0 or 1.0, and avoiding the redundant float conversion.
| try: | |
| price = float(row['price']) | |
| outcome = int(row['outcome']) | |
| except (TypeError, ValueError) as exc: | |
| raise click.ClickException( | |
| f'Invalid price/outcome at CSV row {row_number}' | |
| ) from exc | |
| if not 0 < price < 1: | |
| raise click.ClickException( | |
| f'Invalid price at CSV row {row_number}: expected value in (0, 1)' | |
| ) | |
| if outcome not in (0, 1): | |
| raise click.ClickException( | |
| f'Invalid outcome at CSV row {row_number}: expected 0 or 1' | |
| ) | |
| parsed = {'price': price, 'outcome': float(outcome)} | |
| try: | |
| price = float(row['price']) | |
| outcome = float(row['outcome']) | |
| except (TypeError, ValueError) as exc: | |
| raise click.ClickException( | |
| f'Invalid price/outcome at CSV row {row_number}' | |
| ) from exc | |
| if not 0 < price < 1: | |
| raise click.ClickException( | |
| f'Invalid price at CSV row {row_number}: expected value in (0, 1)' | |
| ) | |
| if outcome not in (0.0, 1.0): | |
| raise click.ClickException( | |
| f'Invalid outcome at CSV row {row_number}: expected 0 or 1' | |
| ) | |
| parsed = {'price': price, 'outcome': outcome} |
| covariates = None | ||
| covariate_names = None | ||
| initial_beta: list[float] = [platform_priors[platform]] | ||
| if hierarchical: | ||
| optional_columns = ('volume', 'duration_hours', 'spread') | ||
| missing = [ | ||
| name for name in optional_columns if len(rows[name]) != len(rows['price']) | ||
| ] | ||
| if missing: | ||
| raise click.ClickException( | ||
| '--hierarchical requires complete CSV column(s): ' + ', '.join(missing) | ||
| ) | ||
| covariates = mle.build_design_matrix( | ||
| volumes=rows['volume'], | ||
| durations_hours=rows['duration_hours'], | ||
| prices=rows['price'], | ||
| spreads=rows['spread'], | ||
| ) | ||
| covariate_names = getattr(mle, '_last_covariate_names', None) | ||
| initial_beta.extend([0.0] * (len(covariate_names or []) - 1)) | ||
|
|
||
| result = mle.fit( | ||
| prices=rows['price'], | ||
| outcomes=rows['outcome'], | ||
| covariates=covariates, | ||
| covariate_names=covariate_names, | ||
| initial_beta=initial_beta, | ||
| ) |
There was a problem hiding this comment.
The CLI command currently accesses the private attribute _last_covariate_names of the WangMLE instance to determine the number of covariates and construct the initial_beta list. This violates encapsulation and creates a tight coupling with the internal implementation details of WangMLE.
Instead, we can determine the number of covariates directly from the shape of the returned covariates design matrix (covariates.shape[1]). Furthermore, mle.fit automatically falls back to using self._last_covariate_names if covariate_names is not provided, so we do not need to fetch or pass covariate_names at all.
covariates = None
initial_beta: list[float] = [platform_priors[platform]]
if hierarchical:
optional_columns = ('volume', 'duration_hours', 'spread')
missing = [
name for name in optional_columns if len(rows[name]) != len(rows['price'])
]
if missing:
raise click.ClickException(
'--hierarchical requires complete CSV column(s): ' + ', '.join(missing)
)
covariates = mle.build_design_matrix(
volumes=rows['volume'],
durations_hours=rows['duration_hours'],
prices=rows['price'],
spreads=rows['spread'],
)
initial_beta.extend([0.0] * (covariates.shape[1] - 1))
result = mle.fit(
prices=rows['price'],
outcomes=rows['outcome'],
covariates=covariates,
initial_beta=initial_beta,
)| def test_calibrate_cli_json_outputs_mle_result(tmp_path): | ||
| csv_path = tmp_path / 'contracts.csv' | ||
| _write_calibration_csv(csv_path) | ||
|
|
||
| result = CliRunner().invoke(cli, ['calibrate', '--csv', str(csv_path), '--json']) | ||
|
|
||
| assert result.exit_code == 0, result.output | ||
| payload = json.loads(result.output) | ||
| assert 'beta' in payload | ||
| assert isinstance(payload['beta'], list) | ||
| assert payload['n_obs'] == 50 |
There was a problem hiding this comment.
The test suite currently lacks coverage for the --hierarchical option and its associated error handling (e.g., when optional columns are missing). Adding tests for these paths ensures that the design matrix construction, initial beta extension, and fitting work correctly without regressions.
def test_calibrate_cli_json_outputs_mle_result(tmp_path):
csv_path = tmp_path / 'contracts.csv'
_write_calibration_csv(csv_path)
result = CliRunner().invoke(cli, ['calibrate', '--csv', str(csv_path), '--json'])
assert result.exit_code == 0, result.output
payload = json.loads(result.output)
assert 'beta' in payload
assert isinstance(payload['beta'], list)
assert payload['n_obs'] == 50
def test_calibrate_cli_hierarchical(tmp_path):
csv_path = tmp_path / 'contracts_hierarchical.csv'
with open(csv_path, 'w', newline='', encoding='utf-8') as handle:
writer = csv.DictWriter(
handle,
fieldnames=['price', 'outcome', 'volume', 'duration_hours', 'spread'],
)
writer.writeheader()
for index in range(50):
price = 0.2 + (index % 30) / 50
outcome = 1 if index % 3 != 0 else 0
writer.writerow({
'price': f'{price:.3f}',
'outcome': outcome,
'volume': 1000.0 + index * 100,
'duration_hours': 24.0 + index,
'spread': 0.02,
})
result = CliRunner().invoke(cli, ['calibrate', '--csv', str(csv_path), '--hierarchical'])
assert result.exit_code == 0, result.output
assert 'constant' in result.output
assert 'ln(1+volume)' in result.output
def test_calibrate_cli_hierarchical_missing_columns(tmp_path):
csv_path = tmp_path / 'contracts.csv'
_write_calibration_csv(csv_path)
result = CliRunner().invoke(cli, ['calibrate', '--csv', str(csv_path), '--hierarchical'])
assert result.exit_code != 0
assert 'requires complete CSV column(s)' in result.outputAddress review feedback by accepting float-formatted binary outcomes, avoiding private WangMLE attributes for hierarchical setup, and covering hierarchical CLI success and missing-column errors. Constraint: PR review requested robust CSV parsing, no private attribute coupling, and hierarchical test coverage. Rejected: reading WangMLE._last_covariate_names from the CLI | it couples the command to private estimator state. Confidence: high Scope-risk: narrow Directive: keep calibrate CLI CSV parsing tolerant of common pandas-style numeric strings. Tested: uv run --python 3.12 --with click --with rich --with numpy --with scipy --with pytest --with pytest-cov --with pytest-asyncio --with pydantic --with requests --with beautifulsoup4 --with feedparser --with httpx --with openai --with openai-agents --with py-clob-client --with tenacity --with textual --with cryptography --with solana --with solders --with kalshi-python python -m pytest tests\\test_calibrate_cli.py -q Tested: uv run --python 3.12 --with ruff ruff check oracle3\\cli\\cli.py tests\\test_calibrate_cli.py Tested: git diff --check Not-tested: full repository pytest/mypy.
|
Addressed the review feedback in cf0531f:
Validation rerun:
|
Summary
oracle3 calibrate --csv FILEfor offline Wang Transform MLE calibrationCloses #33
Validation
uv run --python 3.12 --with ruff ruff check oracle3\cli\cli.py tests\test_calibrate_cli.pyuv run --python 3.12 --with click --with rich --with numpy --with scipy --with pytest --with pytest-cov --with pytest-asyncio --with pydantic --with requests --with beautifulsoup4 --with feedparser --with httpx --with openai --with openai-agents --with py-clob-client --with tenacity --with textual --with cryptography --with solana --with solders --with kalshi-python python -m pytest tests\test_calibrate_cli.py -qgit diff --checkNotes
I used Python's standard
csv.DictReaderrather than addingpandasas a direct dependency becausepyproject.tomldoes not currently declarepandas.