Fix mathematical error in confidence interval calculation#1
Fix mathematical error in confidence interval calculation#1RektPunk wants to merge 1 commit intoxRiskLab:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the confidence interval computation for Pearson residuals to use the quantile as a linear multiplier of the standard error, and updates Ruff configuration keys to the current schema. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider clarifying in the function (e.g., via parameter name or a short comment) whether
q_alphais expected to be a z-score, t-quantile, or something else, so its intended scaling in the interval formula is unambiguous to callers. - The switch to
lint.select/lint.ignoreunder[tool.ruff]assumes a newer Ruff config style; double-check that this matches the Ruff version used in this project, otherwise these options may be ignored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider clarifying in the function (e.g., via parameter name or a short comment) whether `q_alpha` is expected to be a z-score, t-quantile, or something else, so its intended scaling in the interval formula is unambiguous to callers.
- The switch to `lint.select` / `lint.ignore` under `[tool.ruff]` assumes a newer Ruff config style; double-check that this matches the Ruff version used in this project, otherwise these options may be ignored.
## Individual Comments
### Comment 1
<location> `pearsonify/utils.py:4` </location>
<code_context>
import numpy as np
+
def compute_pearson_residuals(y_true, y_pred_proba):
"""Compute Pearson residuals for binary classification."""
y_pred_proba = np.clip(y_pred_proba, 1e-10, 1 - 1e-10)
</code_context>
<issue_to_address>
**issue (bug_risk):** Align the statistical meaning of `q_alpha` with its new usage or rename it for clarity.
The old CI used `np.sqrt(q_alpha * p * (1 - p))`, consistent with `q_alpha` as a chi-square–style factor under the square root. The new form `p ± q_alpha * sqrt(p(1-p))` treats `q_alpha` as a z-score. If callers still pass values tuned for the old meaning, intervals will be incorrectly scaled. Please either update call sites/docs to reflect the z-score interpretation or rename the parameter (e.g., `z_alpha`) to make the change explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import numpy as np | ||
|
|
||
|
|
||
| def compute_pearson_residuals(y_true, y_pred_proba): |
There was a problem hiding this comment.
issue (bug_risk): Align the statistical meaning of q_alpha with its new usage or rename it for clarity.
The old CI used np.sqrt(q_alpha * p * (1 - p)), consistent with q_alpha as a chi-square–style factor under the square root. The new form p ± q_alpha * sqrt(p(1-p)) treats q_alpha as a z-score. If callers still pass values tuned for the old meaning, intervals will be incorrectly scaled. Please either update call sites/docs to reflect the z-score interpretation or rename the parameter (e.g., z_alpha) to make the change explicit.
|
Hi @xRiskLab , I just wanted to follow up on this PR. I know you're likely busy, but I’d appreciate it if you could take a look when you have a moment. Let me know if there’s anything I should clarify or update. Thanks! |
Description
This PR addresses a mathematical inconsistency in the
compute_confidence_intervalsfunction.Previously, the code applied a square root to the quantile value (
q_alpha) when calculating the interval bounds.However, based on the definition of Pearson residuals:$r_i = \frac{y_i - \hat{p}_i}{\sqrt{\hat{p}_i(1 - \hat{p}_i)}}$ $\hat{p}_i \pm q_a \sqrt{\hat{p}_i(1 - \hat{p}_i)}$
To satisfy the condition , the bounds should be calculated as:
Key Changes
compute_confidence_intervalsto useq_alpha * np.sqrt(...)instead ofnp.sqrt(q_alpha * ...).Summary by Sourcery
Correct the computation of confidence intervals based on Pearson residuals and update linter configuration to the current Ruff schema.
Bug Fixes:
Enhancements:
Build: