Conversation
updates: - https://github.com/psf/black → https://github.com/psf/black-pre-commit-mirror - [github.com/psf/black-pre-commit-mirror: 25.1.0 → 25.11.0](psf/black-pre-commit-mirror@25.1.0...25.11.0) - [github.com/pycqa/isort: 6.0.1 → 7.0.0](PyCQA/isort@6.0.1...7.0.0)
…e computation of difference of two proportions confidence intervals using wang method
There was a problem hiding this comment.
Pull request overview
This PR aims to fix issues in LaTeX output generation for make_risk_report, and also includes updates to the Wang (“exactCIdiff”) CI implementation plus supporting test/tooling changes.
Changes:
- Add LaTeX escaping and rebuild the LaTeX table header in
make_risk_report, and raise on unsupportedreturn_type. - Adjust Wang CI internals (stable sorting, plateau handling, output conversion) and relax related test assertions.
- Update developer tooling (flake8 ignores, devcontainer post-create setup) and document fixes in the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
diff_binom_confint/_applications.py |
Adds LaTeX escaping helper, rewrites LaTeX header generation, fixes use of diff_method, adds unsupported return_type error. |
test/test_applications.py |
Adds a test asserting unsupported return_type raises ValueError. |
diff_binom_confint/_specials/_wang.py |
Forwards verbose, uses stable sorts, refines plateau/bounds logic, tweaks output conversion. |
test/test_specials.py |
Adds warning-based fallback to looser tolerances for Wang CI comparisons. |
CHANGELOG.rst |
Records the fixes for Wang edge cases and risk report LaTeX generation. |
.pre-commit-config.yaml |
Broadens flake8 ignore list to include E251/E202. |
.devcontainer/devcontainer.json |
Adds a post-create command to install dev deps and pre-commit hooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| \begin{{tabular}}{{@{{\extracolsep{{6pt}}}}llllllll@{{}}}} | ||
| \toprule | ||
| \multicolumn{{2}}{{l}}{{Feature}} & | ||
| \multicolumn{{3}}{{l}}{{Affected}} & | ||
| \multicolumn{{2}}{{l}}{{{risk_name} Risk (${int(conf_level*100)}\%$ CI)}} & | ||
| {risk_name} Risk Difference (${int(conf_level*100)}\%$ CI) \\ | ||
| \cmidrule(lr){{1-2}}\cmidrule(lr){{3-5}}\cmidrule(lr){{6-7}}\cmidrule(lr){{8-8}} | ||
| & & n & \% & t/v & n & \% & \\ \midrule |
There was a problem hiding this comment.
The generated LaTeX header uses \toprule, \midrule, and \cmidrule, which require the LaTeX booktabs package. Since make_risk_report(..., return_type='latex') returns only the tabular environment, this introduces an implicit dependency that may cause compilation failures for consumers not already using booktabs. Either avoid booktabs commands (use \hline/\cline), or document this requirement in the function docstring/output contract.
| body = "\n".join(_latex_escape(line) for line in body_lines[5:-1]) | ||
|
|
There was a problem hiding this comment.
body_lines[5:-1] relies on a hard-coded pandas DataFrame.to_latex() line layout. That layout varies across pandas versions and options (e.g., whether \midrule is emitted when header=False), so this slice can accidentally drop data rows or include rule lines unexpectedly. Prefer locating/removing wrapper/rule lines by content (e.g., stripping \begin{tabular}, \toprule, \midrule, \bottomrule, \end{tabular}) or generating the body from df_risk_table.iloc[2:] directly instead of slicing by index.
| body = "\n".join(_latex_escape(line) for line in body_lines[5:-1]) | |
| # Keep only data rows from the pandas-generated LaTeX, dropping wrapper/rule lines | |
| filtered_body_lines = [] | |
| for line in body_lines: | |
| stripped = line.strip() | |
| if not stripped: | |
| continue | |
| if stripped.startswith(r"\begin{tabular}"): | |
| continue | |
| if stripped.startswith(r"\end{tabular}"): | |
| continue | |
| if stripped.startswith(r"\toprule"): | |
| continue | |
| if stripped.startswith(r"\midrule"): | |
| continue | |
| if stripped.startswith(r"\bottomrule"): | |
| continue | |
| filtered_body_lines.append(_latex_escape(line)) | |
| body = "\n".join(filtered_body_lines) |
| def binomial_ci_one_sided( | ||
| n_positive: int, | ||
| n_total: int, | ||
| ref_positive: int, | ||
| ref_total: int, | ||
| conf_level: float, | ||
| sides: str, | ||
| precision: float, | ||
| grid_one: int, | ||
| grid_two: int, | ||
| verbose: bool = False, | ||
| ) -> List[float]: |
There was a problem hiding this comment.
binomial_ci_one_sided now accepts a verbose parameter, but it is not used anywhere inside the function body (there are no debug prints/branches). This adds API surface without effect. Either remove the parameter from binomial_ci_one_sided (and the forwarded arguments) or implement verbose-controlled debug logging where needed.
| - id: flake8 | ||
| additional_dependencies: [pycodestyle>=2.11.0] | ||
| args: [--max-line-length=128, '--exclude=./.*,build,dist', '--ignore=E501,W503,E231,E203', --count, --statistics, --show-source] | ||
| args: [--max-line-length=128, '--exclude=./.*,build,dist', '--ignore=E501,W503,E231,E203,E251,E202,E226', --count, --statistics, --show-source] |
There was a problem hiding this comment.
The flake8 config change globally ignores E251/E202, which reduces lint coverage repo-wide and can hide real formatting issues. If this was only needed for a small number of lines, prefer fixing the formatting or using targeted # noqa: E251 / # noqa: E202 on the specific lines triggering it instead of disabling the checks globally.
| args: [--max-line-length=128, '--exclude=./.*,build,dist', '--ignore=E501,W503,E231,E203,E251,E202,E226', --count, --statistics, --show-source] | |
| args: [--max-line-length=128, '--exclude=./.*,build,dist', '--ignore=E501,W503,E231,E203,E226', --count, --statistics, --show-source] |
| else: | ||
| ci = binomial_ci_one_sided( | ||
| n_positive, n_total, ref_positive, ref_total, conf_level, sides_val, precision, grid_one, grid_two | ||
| n_positive, n_total, ref_positive, ref_total, conf_level, sides_val, precision, grid_one, grid_two, verbose | ||
| ) | ||
| return ConfidenceInterval(ci[1], ci[2], ci[0], conf_level, "wang", sides_val) |
There was a problem hiding this comment.
The PR description focuses on make_risk_report LaTeX generation, but this PR also changes the Wang CI implementation (_specials/_wang.py), adjusts test tolerances, and loosens flake8 ignores. Please update the PR description (or split into separate PRs) so reviewers can evaluate the additional changes explicitly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20 +/- ##
==========================================
+ Coverage 97.17% 97.19% +0.01%
==========================================
Files 9 9
Lines 1062 1069 +7
==========================================
+ Hits 1032 1039 +7
Misses 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix errors in generating latex table for the risk report (function
make_risk_report):