feat(proportion.single.inequality): add exact test#334
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces exact binomial test power and sample size calculations for one-sample proportion tests. The feedback identifies several critical issues: the signature change in the _power function breaks existing calls in other modules, and the solve_size implementation for exact tests is incomplete, lacking both a return statement and the logic to determine the minimum sample size. Furthermore, the power calculations for lower and two-sided tails are currently anti-conservative and require adjustments to the critical values. Finally, the documentation for the new exact test feature needs to be populated with mathematical background.
| alpha: float, | ||
| phat: bool, | ||
| continuity_correction: bool, | ||
| exact: bool, |
There was a problem hiding this comment.
Adding exact as a required positional argument to _power breaks existing calls in solve_null_proportion and solve_proportion, which currently only pass 7 arguments. Consider providing a default value for exact to maintain backward compatibility and fix the TypeError in those functions.
| exact: bool, | |
| exact: bool = False, |
| if exact: | ||
| initial_size = solve_size(null_proportion, proportion, alternative, alpha, power, exact=False) | ||
| search_interval_lower_bound = max(floor(0.5 * initial_size), 1) | ||
| search_interval_upper_bound = min(ceil(initial_size * 1.5), SAMPLE_SIZE_SEARCH_MAX) | ||
|
|
||
| return ceil(brentq(func, 0.000001, SAMPLE_SIZE_SEARCH_MAX)) | ||
| rng = range(search_interval_lower_bound, search_interval_upper_bound) | ||
| dict_power = dict.fromkeys(rng) | ||
| for size in rng: | ||
| dict_power[size] = solve_power(null_proportion, proportion, size, alternative, alpha, exact) | ||
| else: | ||
| return ceil(brentq(func, 0.000001, SAMPLE_SIZE_SEARCH_MAX)) |
There was a problem hiding this comment.
The implementation for exact=True in solve_size is incomplete and contains a critical bug: it lacks a return statement, causing the function to return None. Additionally, the current logic populates a dictionary but does not use it to find the minimum sample size. The search logic should iterate through the range and return the smallest size that achieves the target power. Also, ensure that exact is passed correctly to solve_power (it is currently being passed as the 6th positional argument, which corresponds to phat).
| if exact: | |
| initial_size = solve_size(null_proportion, proportion, alternative, alpha, power, exact=False) | |
| search_interval_lower_bound = max(floor(0.5 * initial_size), 1) | |
| search_interval_upper_bound = min(ceil(initial_size * 1.5), SAMPLE_SIZE_SEARCH_MAX) | |
| return ceil(brentq(func, 0.000001, SAMPLE_SIZE_SEARCH_MAX)) | |
| rng = range(search_interval_lower_bound, search_interval_upper_bound) | |
| dict_power = dict.fromkeys(rng) | |
| for size in rng: | |
| dict_power[size] = solve_power(null_proportion, proportion, size, alternative, alpha, exact) | |
| else: | |
| return ceil(brentq(func, 0.000001, SAMPLE_SIZE_SEARCH_MAX)) | |
| if exact: | |
| initial_size = solve_size(null_proportion, proportion, alternative, alpha, power, exact=False) | |
| search_interval_lower_bound = max(floor(0.5 * initial_size), 1) | |
| search_interval_upper_bound = min(ceil(initial_size * 1.5), SAMPLE_SIZE_SEARCH_MAX) | |
| for size in range(search_interval_lower_bound, search_interval_upper_bound + 1): | |
| if solve_power(null_proportion, proportion, size, alternative, alpha, phat, continuity_correction, exact) >= power: | |
| return size | |
| return search_interval_upper_bound | |
| else: | |
| return ceil(brentq(func, 0.000001, SAMPLE_SIZE_SEARCH_MAX)) |
| z_alpha = binom.ppf(alpha, size, null_proportion) | ||
| power = binom.cdf(z_alpha, size, proportion) |
There was a problem hiding this comment.
The power calculation for the lower tail is anti-conservative. binom.ppf(alpha, ...) returns the smallest
| z_alpha = binom.ppf(alpha, size, null_proportion) | |
| power = binom.cdf(z_alpha, size, proportion) | |
| z_alpha = binom.ppf(alpha, size, null_proportion) | |
| if binom.cdf(z_alpha, size, null_proportion) > alpha: | |
| z_alpha -= 1 | |
| power = binom.cdf(z_alpha, size, proportion) |
| z_alpha_2 = binom.ppf(alpha / 2, size, null_proportion) | ||
| power = 1 - binom.cdf(z_alpha_1, size, proportion) + binom.cdf(z_alpha_2, size, proportion) |
There was a problem hiding this comment.
Similar to the one-sided lower tail, the two-sided test implementation might be anti-conservative because z_alpha_2 (the lower critical value) is calculated using ppf without checking if the resulting CDF exceeds
| z_alpha_2 = binom.ppf(alpha / 2, size, null_proportion) | |
| power = 1 - binom.cdf(z_alpha_1, size, proportion) + binom.cdf(z_alpha_2, size, proportion) | |
| z_alpha_2 = binom.ppf(alpha / 2, size, null_proportion) | |
| if binom.cdf(z_alpha_2, size, null_proportion) > alpha / 2: | |
| z_alpha_2 -= 1 | |
| power = 1 - binom.cdf(z_alpha_1, size, proportion) + binom.cdf(z_alpha_2, size, proportion) |
| n = \frac{n'}{4} \left( 1 + \sqrt{1 + \frac{2}{|p_1 - p_0| n'}}\right)^2 | ||
| $$ | ||
|
|
||
| ## 精确检验 {exact-test} |
b5b7cab to
247b762
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request does not contain any code changes or a description, despite the title indicating a new feature implementation for 'proportion.single.inequality'. The acceptance criterion to implement an exact test calculation for single proportion inequality is currently unaddressed. Because no files were modified, the PR is effectively empty and cannot be merged in its current state.
About this PR
- The pull request contains no code changes. Please ensure the intended commits are pushed and included in the branch.
- The PR description is empty. Please provide context, methodology, and any relevant references for the exact test being implemented.
Test suggestions
- The function correctly calculates the p-value for a standard binomial distribution case.
- The function handles edge cases where the number of successes is 0 or equal to the total number of trials.
- The function returns appropriate errors or values for invalid inputs (e.g., negative counts or proportions outside [0, 1])
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. The function correctly calculates the p-value for a standard binomial distribution case.
2. The function handles edge cases where the number of successes is 0 or equal to the total number of trials.
3. The function returns appropriate errors or values for invalid inputs (e.g., negative counts or proportions outside [0, 1])
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 4 high |
🟢 Metrics 4 complexity
Metric Results Complexity 4
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
247b762 to
bef034c
Compare
No description provided.