Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates tie-handling semantics in win-table construction, expands test coverage around those behaviors, and adds supporting release artifacts (NOTICE, example notebook, dev dependency updates).
Changes:
- Adjust paired/local-ROPE win-table construction and tie-solver behavior (
spread/add/forget) and add targeted tests. - Update PyMC model input typing/casting for win counts.
- Add a Critical Difference Diagram example notebook, a NOTICE file for the reference implementation, and add
graphvizto dev dependencies (with lockfile updates).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
bbttest/bbt/alg.py |
Changes tie-solver behavior (spread now assigns 0.5 per tie) and adjusts paired local-ROPE table construction. |
bbttest/bbt/model.py |
Updates win1/win2 typing and casts n/w1/w2 to int32 inside the PyMC model builder. |
bbttest/bbt/_utils.py |
Updates _validate_params to allow extra kwargs when the function declares **kwargs. |
tests/bbt/test_alg.py |
Adds tests for paired local-ROPE path and explicit tie-solver semantics for spread/add/forget. |
tests/bbt/test__utils.py |
Adds a test ensuring _validate_params ignores unexpected kwargs when **kwargs is present. |
examples/02_critical_difference_diagram.ipynb |
Adds an example notebook demonstrating CDD plotting with PyBBT. |
pyproject.toml |
Adds graphviz to the dev dependency group. |
uv.lock |
Lockfile update to include graphviz. |
NOTICE.md |
Adds NOTICE content for the referenced bbtcomp implementation/paper. |
Comments suppressed due to low confidence (1)
bbttest/bbt/alg.py:124
_solve_tiesnow produces fractional win counts fortie_solver="spread"(e.g., 0.5 per tie). Downstream,_mcmcbbt_pymc/_build_bbt_modeluses a Binomial likelihood that requires integernand integerobservedwins, and it currently truncateswin1/win2to ints. As a result,spreadwill silently drop the 0.5 contributions (and can also makeninconsistent with the intended tie semantics). Consider either (a) converting fractional tables to an equivalent integer representation before sampling (e.g., scalew1/w2/nby 2 when halves are present), or (b) rejectingspread/fractional tables when using the Binomial model and requiringtie_solver="davidson"for tie-aware sampling.
def _solve_ties(table: np.ndarray, tie_solver: str) -> np.ndarray:
if tie_solver == "davidson":
return table
if tie_solver == "spread":
tie_val = table[:, TIE_COL] / 2
elif tie_solver == "add":
tie_val = table[:, TIE_COL]
else:
tie_val = 0
table[:, ALG1_COL] += tie_val
table[:, ALG2_COL] += tie_val
return table
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.