Feat/bootstrapping - #5
Conversation
- Add bootstrap.py with state-of-the-art bootstrap resampling - Implement BCa (Bias-Corrected and Accelerated) confidence intervals - Support multiple bootstrap methods: bca, percentile, basic - Add CLI for analyzing metric CSV files - Support mean, median, std, min, max, iqr statistics - Add comprehensive test suite (19 tests) - Update requirements with scipy, pandas, tabulate dependencies - Export bootstrap functions from package __init__.py Usage: python -m ensemble_metrics.bootstrap --metrics-dir ./metrics_all python -m ensemble_metrics.bootstrap --metric-file ./ace.csv --ci-level 0.99
- Add compare_methods() for statistical comparison of two ensembling strategies
- Implement paired bootstrap test for mean differences
- Compute Cohen's d effect size with interpretation
- Add p-values and significance indicators
- Create compare.py CLI for easy command-line comparisons
- Support markdown, LaTeX, and plain text output formats
- Export comparison functions from package
Usage:
python -m src.ensemble_metrics.compare \
--method-a ./baseline/metrics \
--method-b ./proposed/metrics \
--name-a 'Baseline' --name-b 'Proposed'
![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: 83a6757614
ℹ️ 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".
| # Filter out summary rows (mean, std, etc.) | ||
| summary_keywords = ["mean", "std", "median", "min", "max", "sum", "count"] | ||
| mask = ~df["case_id"].str.lower().isin(summary_keywords) | ||
| df_cases = df[mask].copy() |
There was a problem hiding this comment.
The summary-row filter assumes case_id is a string and calls .str.lower(). If a metrics CSV uses numeric case IDs (common in some datasets), pd.read_csv will infer an integer dtype and this line raises “Can only use .str accessor with string values,” which aborts all bootstrap workflows for such files. Converting case_id to string before .str (e.g., astype(str)) avoids the crash while still filtering summary rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds bootstrap-based confidence intervals and paired method comparison utilities (with CLIs) to the ensemble_metrics package.
Changes:
- Introduces a bootstrap resampling module supporting percentile/basic/BCa CIs plus metric CSV aggregation and table formatting.
- Adds a CLI for paired bootstrap comparisons between two metric directories and renders summary tables.
- Adds pytest coverage for the bootstrap workflow and updates public exports + dependencies.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/ensemble_metrics/bootstrap.py |
Implements bootstrap CIs, directory/file processing, and paired method comparison utilities (plus a CLI entrypoint). |
src/ensemble_metrics/compare.py |
Adds a dedicated CLI for comparing two methods’ metric outputs. |
src/ensemble_metrics/__init__.py |
Re-exports bootstrap and comparison APIs from the top-level package. |
tests/test_bootstrap.py |
Adds tests for bootstrap statistics, CSV loading, directory processing, formatting, and a basic integration workflow. |
requirements.txt |
Adds dependencies needed for the new functionality (SciPy/Pandas/Tabulate). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (difference, ci_lower, ci_upper, p_value) | ||
| """ | ||
| if random_state is not None: | ||
| np.random.seed(random_state) |
There was a problem hiding this comment.
paired_bootstrap_test also sets the global NumPy RNG with np.random.seed(random_state), which has the same global-state/concurrency issues as in bootstrap_statistic. Use a local generator scoped to the function to keep randomness isolated and reproducible.
| np.random.seed(random_state) | |
| rng = np.random.default_rng(random_state) | |
| else: | |
| rng = np.random.default_rng() |
|
|
||
| Statistical Tests Performed: | ||
| - Paired bootstrap test for mean difference | ||
| - 95% BCa confidence interval for the difference |
There was a problem hiding this comment.
The CLI help text states that it computes a “95% BCa confidence interval for the difference”, but paired_bootstrap_test currently uses a simple percentile interval (no BCa adjustment). Either update the help text to reflect the implemented method or implement a BCa interval for paired differences so the CLI documentation matches behavior.
| - 95% BCa confidence interval for the difference | |
| - 95% percentile bootstrap confidence interval for the difference |
| numerator = z0 + z_alpha | ||
| denominator = 1 - a * (z0 + z_alpha) | ||
| if np.abs(denominator) < 1e-10: | ||
| return z_alpha # Fall back to unadjusted |
There was a problem hiding this comment.
In _bca_interval.adjusted_percentile, the fallback path returns z_alpha (a z-score), but the caller treats the return value as a probability/percentile in [0, 1]. This causes p_lower/p_upper to be clipped to 0.001/0.999 and can yield incorrect or overly extreme BCa intervals when denominator is near zero. Return the unadjusted percentile (i.e., a probability) instead of the z-score in the fallback branch.
| return z_alpha # Fall back to unadjusted | |
| # Fall back to the unadjusted percentile (probability), not the z-score | |
| return stats.norm.cdf(z_alpha) |
|
|
||
| # Filter out summary rows (mean, std, etc.) | ||
| summary_keywords = ["mean", "std", "median", "min", "max", "sum", "count"] | ||
| mask = ~df["case_id"].str.lower().isin(summary_keywords) |
There was a problem hiding this comment.
load_metric_csv assumes case_id is a string column (df["case_id"].str.lower()), which will raise if the column contains non-strings/NaNs. Consider normalizing first (e.g., astype(str) and/or fillna("")) before calling .str.lower() so CSVs with numeric or missing case IDs fail gracefully or still load.
| mask = ~df["case_id"].str.lower().isin(summary_keywords) | |
| case_id_normalized = df["case_id"].astype(str).str.lower() | |
| mask = ~case_id_normalized.isin(summary_keywords) |
| import pandas as pd | ||
|
|
||
| from .bootstrap import ( | ||
| compare_methods, | ||
| format_comparison_table, | ||
| ) |
There was a problem hiding this comment.
pandas is imported but not used in this module. Removing the unused import will avoid lint warnings and slightly reduce import overhead.
| import os | ||
| import tempfile | ||
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
These imports (os, tempfile, Path) are unused in this test module. Removing them will avoid lint warnings and keep the test focused on the relevant dependencies.
| import os | |
| import tempfile | |
| from pathlib import Path |
| def compare_methods( | ||
| metrics_dir_a: str, | ||
| metrics_dir_b: str, | ||
| method_a_name: str = "Method A", | ||
| method_b_name: str = "Method B", | ||
| n_bootstrap: int = 10000, | ||
| ci_level: float = 0.95, | ||
| random_state: Optional[int] = None, | ||
| metric_pattern: str = "*.csv" | ||
| ) -> pd.DataFrame: |
There was a problem hiding this comment.
The new comparison workflow (paired_bootstrap_test / compare_methods / format_comparison_table) is not covered by tests (no references found under tests/). Adding unit/integration tests for at least one happy-path comparison (matching case_ids, known direction of effect, and expected columns in the output DataFrame) would help prevent regressions in the statistical logic and output schema.
| if random_state is not None: | ||
| np.random.seed(random_state) | ||
|
|
There was a problem hiding this comment.
bootstrap_statistic seeds the global NumPy RNG via np.random.seed(random_state), which mutates global state and can make results from other code non-reproducible (and is problematic under concurrency). Prefer using a local RNG (e.g., np.random.default_rng(random_state)) and draw samples from that generator instead of the global np.random module.
| import argparse | ||
| import os | ||
| import glob | ||
| from typing import Dict, List, Optional, Tuple, Union |
There was a problem hiding this comment.
Import of 'Dict' is not used.
Import of 'Union' is not used.
| from typing import Dict, List, Optional, Tuple, Union | |
| from typing import List, Optional, Tuple |
- Use local RNG (np.random.default_rng) instead of global np.random.seed for thread safety and isolated reproducibility - Fix BCa fallback to return probability instead of z-score in _bca_interval.adjusted_percentile - Handle non-string case_id values in load_metric_csv by converting to string before filtering - Update compare.py CLI help text to correctly state 'percentile' confidence interval (not BCa) - Remove unused imports: pandas from compare.py, os/tempfile/Path from test_bootstrap.py
- Update .gitignore to prevent re-committing
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def compare_methods( | ||
| metrics_dir_a: str, | ||
| metrics_dir_b: str, | ||
| method_a_name: str = "Method A", | ||
| method_b_name: str = "Method B", | ||
| n_bootstrap: int = 10000, | ||
| ci_level: float = 0.95, | ||
| random_state: Optional[int] = None, | ||
| metric_pattern: str = "*.csv" | ||
| ) -> pd.DataFrame: |
There was a problem hiding this comment.
New comparison functionality (paired_bootstrap_test, cohens_d, compare_methods, format_comparison_table) is not covered by tests in tests/test_bootstrap.py. Adding unit tests for at least: (1) deterministic behavior under fixed seed, (2) case alignment by case_id, and (3) expected p-value/CI behavior on a simple synthetic dataset would help prevent regressions.
| python -m ensemble_metrics.compare \\ | ||
| --method-a ./results_baseline/metrics_all \\ | ||
| --method-b ./results_new/metrics_all \\ | ||
| --name-a "Baseline" --name-b "New Method" | ||
|
|
||
| # With specific confidence level and output | ||
| python -m ensemble_metrics.compare \\ | ||
| --method-a ./method1/metrics \\ | ||
| --method-b ./method2/metrics \\ | ||
| --ci-level 0.99 \\ | ||
| --output comparison_results.csv | ||
|
|
||
| # LaTeX output for papers | ||
| python -m ensemble_metrics.compare \\ |
There was a problem hiding this comment.
The epilog examples use python -m ensemble_metrics.compare, but this repo’s other CLI usage (e.g., src/ensemble_metrics/README.md) uses python -m src.ensemble_metrics.... Please align the module path in the help text with the actual package/module layout (or add packaging/entrypoints so ensemble_metrics is importable directly).
| python -m ensemble_metrics.compare \\ | |
| --method-a ./results_baseline/metrics_all \\ | |
| --method-b ./results_new/metrics_all \\ | |
| --name-a "Baseline" --name-b "New Method" | |
| # With specific confidence level and output | |
| python -m ensemble_metrics.compare \\ | |
| --method-a ./method1/metrics \\ | |
| --method-b ./method2/metrics \\ | |
| --ci-level 0.99 \\ | |
| --output comparison_results.csv | |
| # LaTeX output for papers | |
| python -m ensemble_metrics.compare \\ | |
| python -m src.ensemble_metrics.compare \\ | |
| --method-a ./results_baseline/metrics_all \\ | |
| --method-b ./results_new/metrics_all \\ | |
| --name-a "Baseline" --name-b "New Method" | |
| # With specific confidence level and output | |
| python -m src.ensemble_metrics.compare \\ | |
| --method-a ./method1/metrics \\ | |
| --method-b ./method2/metrics \\ | |
| --ci-level 0.99 \\ | |
| --output comparison_results.csv | |
| # LaTeX output for papers | |
| python -m src.ensemble_metrics.compare \\ |
| # Pivot for better readability | ||
| pivot_df = df_formatted.pivot( | ||
| index="metric", | ||
| columns="statistic", | ||
| values="value_with_ci" if "value_with_ci" in df_formatted.columns else "point_estimate" | ||
| ) |
There was a problem hiding this comment.
format_results_table uses DataFrame.pivot(...), which raises if there are duplicate (metric, statistic) pairs. Since statistics parsing doesn’t deduplicate and callers can pass duplicates, this can fail at runtime. Consider validating uniqueness up-front (and erroring clearly) or using pivot_table(..., aggfunc='first') if duplicates should be tolerated.
| if len(data_a) != len(data_b): | ||
| raise ValueError(f"Arrays must have same length: {len(data_a)} vs {len(data_b)}") | ||
|
|
||
| n = len(data_a) |
There was a problem hiding this comment.
paired_bootstrap_test doesn’t guard against n < 2, n_bootstrap <= 0, or invalid ci_level. With n == 0 the resampling call will fail, and with small/zero n_bootstrap you can get divide-by-zero or invalid percentiles. Add input validation similar to bootstrap_statistic (and include the observed sizes in the error message).
| n = len(data_a) | |
| n = len(data_a) | |
| if n < 2: | |
| raise ValueError(f"paired_bootstrap_test requires at least 2 paired samples; got n={n}") | |
| if n_bootstrap <= 0: | |
| raise ValueError( | |
| f"paired_bootstrap_test requires n_bootstrap > 0; got n_bootstrap={n_bootstrap} " | |
| f"for n={n}" | |
| ) | |
| if not (0 < ci_level < 1): | |
| raise ValueError( | |
| f"ci_level must be in (0, 1); got ci_level={ci_level} for n={n}, " | |
| f"n_bootstrap={n_bootstrap}" | |
| ) |
| jackknife_stats = np.zeros(n) | ||
| for i in range(n): | ||
| jackknife_sample = np.delete(data, i) | ||
| jackknife_stats[i] = statistic_func(jackknife_sample) | ||
|
|
There was a problem hiding this comment.
_bca_interval assumes statistic_func can be computed on each jackknife sample of size n-1. With the default statistics=['mean','median','std'] and method='bca', small datasets can break: for example, std uses ddof=1, and when n==2 each jackknife sample has length 1 so statistic_func returns NaN, which then propagates to NaN percentiles and raises in np.percentile. Consider validating minimum n for BCa (or for specific statistics), and/or handling exceptions/NaNs in jackknife stats with a fallback (e.g., a=0 or falling back to percentile CI).
| import argparse | ||
| import os | ||
| import glob | ||
| from typing import Dict, List, Optional, Tuple, Union |
There was a problem hiding this comment.
Dict and Union are imported from typing but not used anywhere in this module. Removing unused imports will keep the file cleaner and avoids lint failures if you run a linter.
| from typing import Dict, List, Optional, Tuple, Union | |
| from typing import List, Optional, Tuple |
| f"ci_{int(r.ci_level*100)}_lower": r.ci_lower, | ||
| f"ci_{int(r.ci_level*100)}_upper": r.ci_upper, |
There was a problem hiding this comment.
bootstrap_metrics_directory names CI columns using int(r.ci_level * 100), which truncates non-integer confidence levels (e.g., 0.975 -> 97) and can mislabel results. Consider rounding (e.g., round(...)) or encoding the CI level more precisely in the column name to avoid ambiguity.
| f"ci_{int(r.ci_level*100)}_lower": r.ci_lower, | |
| f"ci_{int(r.ci_level*100)}_upper": r.ci_upper, | |
| f"ci_{int(round(r.ci_level * 100))}_lower": r.ci_lower, | |
| f"ci_{int(round(r.ci_level * 100))}_upper": r.ci_upper, |
| df = pd.read_csv(filepath) | ||
|
|
||
| # Get metric name from columns (second column typically) | ||
| metric_name = [col for col in df.columns if col != "case_id"][0] | ||
|
|
||
| # Filter out summary rows (mean, std, etc.) | ||
| # Convert case_id to string to handle numeric or mixed types | ||
| summary_keywords = ["mean", "std", "median", "min", "max", "sum", "count"] | ||
| df["case_id"] = df["case_id"].fillna("").astype(str) | ||
| mask = ~df["case_id"].str.lower().isin(summary_keywords) | ||
| df_cases = df[mask].copy() |
There was a problem hiding this comment.
load_metric_csv assumes a case_id column exists and that there is at least one other column ([...][0]). If the CSV schema differs, this will raise a KeyError/IndexError with a confusing stack trace. Consider validating the expected columns up-front (presence of case_id and exactly one metric column, or explicitly choosing the metric column) and raising a clear ValueError when the schema is unexpected.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.