fix: align wang method prob2step plateau logic with upstream R implementation - #33
fix: align wang method prob2step plateau logic with upstream R implementation#33wenh06 wants to merge 2 commits into
Conversation
…entation Three regressions introduced in 8267a97 (revert) caused Python/R divergence: 1. _prob2step/_prob2steplmin: Replaced the isclose-based plateau detection with argmax/argmin. On symmetric profiles the old code bracketed both mirror peaks, causing the second-stage grid-refinement to miss the true maximum and systematically underestimate probmid by up to ~7e-3. This cascaded through the binary search to shift confidence bounds by as much as 0.02. argmax/argmin matches R's which(sumofprob == max(sumofprob)) semantics (single-index, ulp-stable breaking of exact ties). 2. argsort: Restored kind="stable" so that tied rows retain the same order as R's stable order(). 3. np.unique: Switched to a return_index + sort(uniq_idx) pattern that preserves first-occurrence order, matching R's unique() behavior instead of NumPy's lexicographic sort. The exact CI-failing case (8,37,23,37) now produces identical results: R (-0.59305, -0.15878) = Python (-0.59305, -0.15878).
There was a problem hiding this comment.
Pull request overview
This PR fixes a numerical divergence between the Python and upstream R implementations of the Wang method (previously causing test_wang_method to fail), by restoring R-equivalent plateau selection and order stability in intermediate table construction.
Changes:
- Replace
_prob2step/_prob2steplminplateau detection withargmax/argmin-based refinement to match R’swhich(... == max/min(...))behavior. - Make sorting deterministic by using
argsort(kind="stable")where R’sorder()stability matters. - Preserve first-occurrence row order when de-duplicating with
np.unique(..., return_index=True)+ index sort, matching R’sunique()semantics. - Fix a list misuse bug in
remove_parameters_returns_from_docstringand make the corresponding test robust across Python versions by normalizing docstrings withinspect.cleandoc.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
diff_binom_confint/_specials/_wang.py |
Align Wang method grid refinement and ordering semantics with upstream R to eliminate CI-level numerical drift. |
diff_binom_confint/_utils.py |
Fix docstring processing bug (indices2remove.extend(...)) to correctly remove trailing parameter/return blocks. |
test/test_utils.py |
Normalize docstring comparisons with inspect.cleandoc to avoid Python-version-dependent indentation differences. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 5ff7fbafe4
ℹ️ 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".
| indices2remove.extend(list(range(start_idx, len(new_doc)))) | ||
| new_doc.extend(["\n", parameters_indicator or returns_indicator]) |
There was a problem hiding this comment.
When the requested block is the last item in a docstring with no following blank/same-indent line, this branch now becomes reachable; after extending indices2remove it also appends synthetic lines, so removing a terminal return item leaves a stray Returns/Parameters section, and removing a terminal parameter from a docstring without a Returns section later appends returns_indent as None and still raises TypeError. Handle the EOF case by just removing the collected range (and not padding with section headers/None) so terminal sections are actually removable.
Useful? React with 👍 / 👎.
…cstring item When a parameter or return value at EOF was the last item in its section, the old code appended a stray "Parameters" / "Returns" header line. Just collect the remaining lines into indices2remove and let the trailing-empty-line cleanup handle the rest.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33 +/- ##
==========================================
+ Coverage 97.19% 97.26% +0.07%
==========================================
Files 9 9
Lines 1069 1062 -7
==========================================
- Hits 1039 1033 -6
+ Misses 30 29 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Fixes the Wang method Python/R divergence reported by CI (
test_wang_methodfailing at loose 1e-2 tolerance).Root Cause
Commit
8267a97(revert) introduced three regressions:_prob2step/_prob2steplminusedisclose-based plateau detection instead ofargmax/argminn_total == ref_total), both mirror peaks entered the plateau, causing the second-stage grid refinement bracket to span the entire symmetric interval and miss the true maximum — probmid systematically underestimated by up to ~7e-3, cascading to CI bound errors up to 0.02argsort()withoutkind="stable"order()np.uniquesorts lexicographicallyunique()preserves first-occurrence orderFix
argmax/argmin— matches R'swhich(sumofprob == max(sumofprob))semanticskind="stable"to bothargsortcallsreturn_index+np.sort(uniq_idx)for order-preserving uniqueVerification
(8,37,23,37)now produces identical results:(-0.59305, -0.15878)on both R and Pythontest_wang_method: all strict (1e-4) checks pass