Support partial-hand deals in CalcDDtable and dtest tooling - #346
Support partial-hand deals in CalcDDtable and dtest tooling#346tameware wants to merge 13 commits into
Conversation
Allows generating deals with 1–13 cards per hand instead of always 13. The play generator now derives the total card count from the deal rather than hard-coding 52. Co-authored-by: Cursor <cursoragent@cursor.com>
Allows regenerating hand lists with fewer than 13 cards per hand. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
They are recreated by regenerate_hand_lists.sh / generate_partial_hands_lists.sh when needed. Co-authored-by: Cursor <cursoragent@cursor.com>
|
There's more to be done here, in particular having the PAR lines make sense. This is ready to merge now, though. It's an improvement over what preceded it, where dtest accepted deals of fewer than 12 cards but the TABLE line was incorrect. If I continue without merging then the PR will get bigger than I'd like. |
There was a problem hiding this comment.
Pull request overview
This PR extends DDS tooling and the DD-table calculation paths to correctly handle partial bridge deals (fewer than 13 cards per hand), and adds scripts/tests to generate and validate those partial-hand deal lists.
Changes:
- Fix DD-table trick conversion and null-window bounds so results are computed out of remaining tricks instead of assuming 13.
- Add
--cardssupport tocreate_list_for_dtestand forward it from hand-list regeneration scripts; add a helper script to generate all partial-hand lists. - Add a system regression test covering CalcDDtable behavior on a 1-card-per-hand deal and wire it into Bazel.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| utilities/src/regenerate_hand_lists.sh | Adds --cards arg parsing and forwards it to the list generator. |
| utilities/src/generate_partial_hands_lists.sh | New helper script to generate hands/partial/* lists for 1–12 cards/hand. |
| python/utilities/src/create_list_for_dtest.py | Adds --cards, generates partial deals, and updates play generation to iterate over total remaining cards. |
| python/utilities/tests/create_list_for_dtest_test.py | Adds unit coverage for --cards parsing and partial dealing behavior. |
| library/src/solver_if.cpp | Bounds solve_same_board search to remaining tricks (not hardcoded 13). |
| library/src/calc_tables.cpp | Computes remaining tricks from holdings; uses it when converting leader-side score into declarer tricks; avoids 13 - score assumptions. |
| library/tests/system/calc_dd_table_partial_test.cpp | New regression test ensuring CalcDDtable variants report tricks out of remaining cards. |
| library/tests/system/BUILD.bazel | Registers the new system test. |
| .gitignore | Ignores generated hands/partial/ outputs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Avoids bash treating quoted expansions from ${CARDS:+...} as literal
characters in the generator arguments.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the |
Fix the generate_partial_hands_lists.sh typo, resolve repo root with quoted paths, and add a main() regression test that PLAY has 4*N cards. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed remaining review comments:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
utilities/src/generate_partial_hands_lists.sh:22
- The final
mvmakes this script non-idempotent (re-running after a successful run will fail if01_card/already exists, and it also fails if a previous run already moved01_cards/). Prefer generating the desired directory name in the loop instead of renaming afterward.
./utilities/src/regenerate_hand_lists.sh \
--cards "$CARDS"
done
mv "${PARENT_DIR}/01_cards/" "${PARENT_DIR}/01_card/"
Use full solve_board only when remaining tricks are fewer than 13 so bulk 13-card CalcDDtable/CalcAllTables keep the fast null-window path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the CalcDDtable performance comment: restored |
Give a clear error when --cards has no value instead of an unbound variable failure under set -u. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed: bare |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
utilities/src/generate_partial_hands_lists.sh:22
- The final
mvis not safe to rerun: if${PARENT_DIR}/01_card/already exists,mv .../01_cards/ .../01_card/will nest the directory (01_card/01_cards) instead of replacing it.
mv "${PARENT_DIR}/01_cards/" "${PARENT_DIR}/01_card/"
utilities/src/regenerate_hand_lists.sh:20
--cardsassumes a following value ($2) without checking it exists. Withset -u, calling the script as... --cardswill abort with an unhelpful “unbound variable” error instead of a clear message.
--cards)
if [[ $# -lt 2 ]]; then
echo "Missing value for --cards (expected 1–13)" >&2
exit 1
Remove any existing 01_card directory before renaming 01_cards so a second run does not nest directories. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the suppressed comments from the latest review:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
library/src/calc_tables.cpp:61
- The legacy C++ convenience wrapper
calc_dd_table*still hardcodes13 - solved...score[...](seelibrary/src/calc_dd_table.cpp:70-72), so partial-hand deals will still return impossible trick counts through that API even though the C APIs in this file now useremaining_tricks_from_holdings(). This leaves the PR’s “Support partial-hand deals in CalcDDtable” incomplete for C++ callers.
auto declarer_tricks_from_leader_score(
int remaining_tricks,
int leader_side_score) -> int
{
return remaining_tricks - leader_side_score;
}
Share remaining-tricks helpers with calc_tables and add a C++ API regression test so partial-hand deals no longer use hardcoded 13 - score. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the suppressed comment: C++ |
Summary
CalcDDtable/ solver trick counting for deals with fewer than 13 cards per hand.--cardstocreate_list_for_dtestand forward it fromregenerate_hand_lists.sh.generate_partial_hands_lists.shand ignore generatedhands/partial/outputs.Test plan
bazelisk test //library/tests/system:calc_dd_table_partial_testbazelisk test //python/utilities:create_list_for_dtest_testMade with Cursor