From a438be8bda517787dc97c773b3dd107970ebc294 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Mon, 5 Oct 2026 12:01:50 +0200 Subject: [PATCH] [Analytics] Return zero instead of NaN for impossible draw probabilities The draw-probability widget computes a hypergeometric probability as a sum of qLn terms. Two argument combinations reach a non-positive term and return NaN, which the results table prints as "nan": - k > K, asking for more copies than the deck holds. A deck with one Forest, draw 7, "exactly 3" gives K=1, k=3. - n - k > N - K, where drawing k targets would need more non-target cards than exist. An 8-card deck with 6 targets, draw 5, "exactly 0" gives N-K=2, n-k=5. Both are reachable: the quantity and drawn spinboxes allow 1-60, and the "at least" branch caps k at copies while the "exactly" branch passes the spinbox value straight through. Extract the function so it can be tested on its own, and guard both cases. Restoring the old guard fails 3 of the 7 new tests. --- cockatrice/CMakeLists.txt | 1 + .../draw_probability_widget.cpp | 21 +---- .../draw_probability_widget.h | 2 +- .../hypergeometric_probability.cpp | 27 ++++++ .../hypergeometric_probability.h | 12 +++ tests/CMakeLists.txt | 9 ++ tests/hypergeometric_probability_test.cpp | 83 +++++++++++++++++++ 7 files changed, 134 insertions(+), 21 deletions(-) create mode 100644 cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.cpp create mode 100644 cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.h create mode 100644 tests/hypergeometric_probability_test.cpp diff --git a/cockatrice/CMakeLists.txt b/cockatrice/CMakeLists.txt index 32b81cf61..06a93e3bf 100644 --- a/cockatrice/CMakeLists.txt +++ b/cockatrice/CMakeLists.txt @@ -201,6 +201,7 @@ set(cockatrice_SOURCES src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_config.cpp src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_config_dialog.cpp src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.cpp + src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.cpp src/interface/widgets/deck_analytics/analyzer_modules/mana_base/mana_base_config.cpp src/interface/widgets/deck_analytics/analyzer_modules/mana_base/mana_base_config_dialog.cpp src/interface/widgets/deck_analytics/analyzer_modules/mana_base/mana_base_widget.cpp diff --git a/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.cpp b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.cpp index 90d9f58ef..2f5b59009 100644 --- a/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.cpp +++ b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.cpp @@ -6,6 +6,7 @@ #include "../../deck_list_statistics_analyzer.h" #include "deck_list_model.h" #include "draw_probability_config_dialog.h" +#include "hypergeometric_probability.h" #include "libcockatrice/card/database/card_database_querier.h" #include "libcockatrice/deck_list/tree/deck_list_card_node.h" @@ -233,23 +234,3 @@ void DrawProbabilityWidget::updateFilterOptions() resultTable->setItem(row, 2, new QTableWidgetItem(QString::number(probability * 100.0, 'f', 2))); } } - -double DrawProbabilityWidget::hypergeometricProbability(int N, int K, int n, int k) -{ - if (k < 0 || k > n || K > N || n > N) { - return 0.0; - } - - double logP = 0.0; - for (int i = 1; i <= k; ++i) { - logP += qLn(double(K - k + i) / i); - } - for (int i = 1; i <= n - k; ++i) { - logP += qLn(double(N - K - (n - k) + i) / i); - } - for (int i = 1; i <= n; ++i) { - logP -= qLn(double(N - n + i) / i); - } - - return qExp(logP); -} diff --git a/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.h b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.h index 7c7bdb50a..759248c2e 100644 --- a/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.h +++ b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/draw_probability_widget.h @@ -3,6 +3,7 @@ #include "../../abstract_analytics_panel_widget.h" #include "draw_probability_config.h" +#include "hypergeometric_probability.h" #include @@ -51,7 +52,6 @@ private: QTableWidget *resultTable; - double hypergeometricProbability(int N, int K, int n, int k); double calculateProbability(int totalCards, int copies, int drawn, bool atLeast); }; diff --git a/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.cpp b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.cpp new file mode 100644 index 000000000..79d4f185c --- /dev/null +++ b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.cpp @@ -0,0 +1,27 @@ +#include "hypergeometric_probability.h" + +#include + +double hypergeometricProbability(int N, int K, int n, int k) +{ + // Every rejected case here would otherwise reach qLn with a non-positive argument and + // produce NaN. Beyond the obvious out-of-range checks, the two that matter are k > K (you + // cannot draw more targets than exist) and n - k > N - K (drawing k targets would need more + // non-target cards than the deck holds). + if (k < 0 || k > n || k > K || K > N || n > N || n - k > N - K) { + return 0.0; + } + + double logP = 0.0; + for (int i = 1; i <= k; ++i) { + logP += qLn(double(K - k + i) / i); + } + for (int i = 1; i <= n - k; ++i) { + logP += qLn(double(N - K - (n - k) + i) / i); + } + for (int i = 1; i <= n; ++i) { + logP -= qLn(double(N - n + i) / i); + } + + return qExp(logP); +} diff --git a/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.h b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.h new file mode 100644 index 000000000..3c56ea9ac --- /dev/null +++ b/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.h @@ -0,0 +1,12 @@ +#ifndef COCKATRICE_HYPERGEOMETRIC_PROBABILITY_H +#define COCKATRICE_HYPERGEOMETRIC_PROBABILITY_H + +/** @brief Probability of drawing exactly k of K target cards in a hand of n from a population of N. + * + * Returns 0.0 for arguments that cannot describe a draw, which includes k > K: you cannot draw + * more target cards than the deck holds, and asking for it yields log of a negative term rather + * than a probability. + */ +double hypergeometricProbability(int N, int K, int n, int k); + +#endif // COCKATRICE_HYPERGEOMETRIC_PROBABILITY_H diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 02ecf7cc5..87dfbfed4 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -274,6 +274,15 @@ cockatrice_add_test( INCLUDE_DIRS ${CMAKE_SOURCE_DIR}/cockatrice/src ) +cockatrice_add_test( + NAME + hypergeometric_probability_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability/hypergeometric_probability.cpp + hypergeometric_probability_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/widgets/deck_analytics/analyzer_modules/draw_probability +) cockatrice_add_test( NAME lag_monitor_test diff --git a/tests/hypergeometric_probability_test.cpp b/tests/hypergeometric_probability_test.cpp new file mode 100644 index 000000000..4b45dc53b --- /dev/null +++ b/tests/hypergeometric_probability_test.cpp @@ -0,0 +1,83 @@ +/** @file hypergeometric_probability_test.cpp + * @brief Tests for the draw-probability hypergeometric calculation. + * @ingroup Tests + */ + +#include +#include +#include +#include + +// Drawing more target cards than the deck holds has no valid outcome. Without a k > K guard the +// running product reaches qLn of a negative term, so the result is NaN and the widget prints "nan". +TEST(HypergeometricProbability, AskingForMoreCopiesThanTheDeckHoldsIsZero) +{ + EXPECT_DOUBLE_EQ(hypergeometricProbability(60, 1, 7, 3), 0.0); + EXPECT_DOUBLE_EQ(hypergeometricProbability(60, 2, 7, 4), 0.0); + EXPECT_DOUBLE_EQ(hypergeometricProbability(4, 1, 1, 2), 0.0); +} + +// Drawing k targets from n cards needs n - k non-targets, and the deck may not hold that many. +// An 8-card deck with 6 targets leaves 2 non-targets, so drawing 5 cards with 0 targets would +// need 5 of them. The n - k term goes negative, which is the same NaN through a different factor. +TEST(HypergeometricProbability, AskingForMoreNonTargetsThanTheDeckHoldsIsZero) +{ + EXPECT_DOUBLE_EQ(hypergeometricProbability(8, 6, 5, 0), 0.0); + EXPECT_DOUBLE_EQ(hypergeometricProbability(8, 6, 5, 1), 0.0); + EXPECT_DOUBLE_EQ(hypergeometricProbability(10, 9, 8, 0), 0.0); +} + +// k == K is the reachable edge of the previous case: drawing every target card exactly. It must +// stay valid, so the guard cannot be written as k >= K. +TEST(HypergeometricProbability, DrawingEveryCopyIsNotRejected) +{ + // One Forest in a 60-card deck, drawing 7: P(the Forest) = 7/60. + EXPECT_NEAR(hypergeometricProbability(60, 1, 7, 1), 7.0 / 60.0, 1e-12); + EXPECT_GT(hypergeometricProbability(60, 4, 7, 4), 0.0); + // The only way to fill a hand from a deck the size of the hand is to take everything. + EXPECT_NEAR(hypergeometricProbability(4, 2, 4, 2), 1.0, 1e-12); +} + +// k == 0 with n == 0 draws nothing at all, which is certain. +TEST(HypergeometricProbability, DrawingAnEmptyHandIsCertain) +{ + EXPECT_NEAR(hypergeometricProbability(60, 4, 0, 0), 1.0, 1e-12); + EXPECT_NEAR(hypergeometricProbability(4, 2, 0, 0), 1.0, 1e-12); +} + +TEST(HypergeometricProbability, ImpossibleDrawsAreZero) +{ + EXPECT_DOUBLE_EQ(hypergeometricProbability(60, 4, 7, 9), 0.0); // k > n + EXPECT_DOUBLE_EQ(hypergeometricProbability(10, 12, 5, 3), 0.0); // K > N + EXPECT_DOUBLE_EQ(hypergeometricProbability(10, 4, 20, 5), 0.0); // n > N + EXPECT_DOUBLE_EQ(hypergeometricProbability(10, 4, 5, -1), 0.0); // k < 0 +} + +// The sweep that motivated both guards: no argument combination may produce NaN. The upper bound +// is 1.0 plus a few ulps, since the sum of logarithms accumulates rounding error on a certain +// outcome rather than landing on exactly 1. +TEST(HypergeometricProbability, ProbabilitiesAreFiniteAndInRange) +{ + for (int N = 1; N <= 70; N += 7) { + for (int K = 0; K <= N; K += 3) { + for (int n = 0; n <= N; n += 5) { + for (int k = 0; k <= n; ++k) { + const double p = hypergeometricProbability(N, K, n, k); + const std::string at = "N=" + std::to_string(N) + " K=" + std::to_string(K) + + " n=" + std::to_string(n) + " k=" + std::to_string(k); + ASSERT_FALSE(std::isnan(p)) << at; + ASSERT_GE(p, 0.0) << at; + ASSERT_LE(p, 1.0 + 1e-12) << at; + } + } + } + } +} + +// A known value: the chance of drawing both copies of a 2-of-4 card in a 7-card hand from a +// 60-card deck. C(4,2)*C(56,5)/C(60,7). +TEST(HypergeometricProbability, KnownValueMatchesTheClosedForm) +{ + const double expected = (6.0 * 3819816.0) / 386206920.0; + EXPECT_NEAR(hypergeometricProbability(60, 4, 7, 2), expected, 1e-9); +}