compute_effsize: honor eftype for one-sample tests (eta_square was out of range) - #519
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 19 19
Lines 3305 3305
Branches 488 488
=======================================
Hits 3251 3251
Misses 32 32
Partials 22 22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks @AlejandroCoronadoN, good catch. See Claude-assisted review below:
The catch is that convert_effsize accepts a narrower set of eftype values than compute_effsize / _check_eftype does, so delegating unconditionally exposes that mismatch: cles / cohen_dz silently become AUC, and r — still documented and supported — now raises. Both leak into pairwise_tests whenever a group reduces to a single observation. Details in the three inline comments; the suggestion on the first one covers all of them.
Two non-blocking notes:
- The Notes section of
compute_effsize(around line 705) still documents the one-sample case as returningd = (X̄ - µ) / σ_Xwith no mention thateftypeis now honored — worth a sentence, plus the caveat thateta_square/odds_ratioassume a two-group contrast. - A CHANGELOG entry is still missing.
(The --doctest-modules failures on CI are pre-existing on main — NumPy 2 np.float64(...) reprs — and unrelated to this PR.)
| # Case 1: One-sample Test | ||
| d = (x.mean() - y) / x.std(ddof=1) | ||
| return d | ||
| return convert_effsize(d, "cohen", eftype, nx=nx, ny=ny) |
There was a problem hiding this comment.
eftype="cles" and eftype="cohen_dz" now silently return an AUC value.
_check_eftype (src/pingouin/utils.py:325) accepts cles and cohen_dz, but convert_effsize has no branch for either, so both fall into the trailing else: # ['auc'] and return norm.cdf(d / sqrt(2)).
>>> pg.compute_effsize([1, 2, 3, 4, 5, 6, 7], y=0, eftype="cles")
0.90478 # identical to eftype="auc"This also leaks into callers: with a single-observation group,
pg.pairwise_tests(data=df, dv="v", between="g", effsize="CLES") reports 1.996e-05 in the CLES
column — an AUC value, plausible-looking and inside [0, 1], so it will not be noticed.
Suggested fix below also covers the two other comments on this line (the r regression and the
ny=1 degeneration), so it can be applied on its own:
| return convert_effsize(d, "cohen", eftype, nx=nx, ny=ny) | |
| if eftype.lower() in ["cohen", "cohen_dz", "r", "cles"]: | |
| # Not supported by convert_effsize: 'r' raises a deprecation error there, | |
| # and 'cles'/'cohen_dz' have no branch and would fall through to AUC. | |
| return d | |
| if eftype.lower() == "pointbiserialr": | |
| # ny=1 degenerates McGrath & Meyer's `a` to nx, which makes the conversion | |
| # sample-size dependent. ny=None uses the intended a = 4. | |
| return convert_effsize(d, "cohen", eftype, nx=nx, ny=None) | |
| return convert_effsize(d, "cohen", eftype, nx=nx, ny=ny) |
| # Case 1: One-sample Test | ||
| d = (x.mean() - y) / x.std(ddof=1) | ||
| return d | ||
| return convert_effsize(d, "cohen", eftype, nx=nx, ny=ny) |
There was a problem hiding this comment.
eftype="r" now raises ValueError in the one-sample branch.
convert_effsize deliberately rejects "r" ("has been deprecated. Please use 'pointbiserialr' instead"), but "r" is still a documented, supported value of compute_effsize (src/pingouin/effsize.py:657) and of pairwise_tests (src/pingouin/pairwise.py:105), and the two-sample branch right below handles it explicitly via pearsonr.
pg.compute_effsize(x, y=0, eftype="r")previously returned the d, now raises.pg.pairwise_tests(data=df, dv="v", between="g", effsize="r")now raises that deprecation error from deep inside whenever a group reduces to one observation (ann=1cell, or one remaining value afternan_policy="pairwise"deletion) — previously it produced a number.
Note that ny == 1 is a size check, not an "is scalar" check, so any length-1 y array takes this path too.
| # Case 1: One-sample Test | ||
| d = (x.mean() - y) / x.std(ddof=1) | ||
| return d | ||
| return convert_effsize(d, "cohen", eftype, nx=nx, ny=ny) |
There was a problem hiding this comment.
Passing ny=1 makes the pointbiserialr conversion sample-size dependent.
convert_effsize computes a = ((nx + ny) ** 2 - 2 * (nx + ny)) / (nx * ny) (line 613), which with ny=1 degenerates to a ~= nx, i.e. r_pb = d / sqrt(d**2 + n) instead of the intended a = 4. For a fixed d = 1.8516:
| n | pointbiserialr |
|---|---|
| 7 | 0.577 |
| 20 | 0.383 |
| 100 | 0.182 |
| 1000 | 0.058 |
A fixed effect size must not map to a correlation that vanishes as the sample grows. Passing ny=None (which falls back to a = 4) or rejecting pointbiserialr on this path both work.
Worth noting that ny=1 is correct for hedges: d * (1 - 3 / (4 * (nx + 1) - 9)) reduces to the standard one-sample J = 1 - 3 / (4n - 5), which is why the 1.6101 value in the PR description checks out. The (d/2) and d * pi / sqrt(3) factors behind eta_square / odds_ratio do assume a two-equal-group contrast, but they stay bounded and monotonic — a documentation caveat rather than a bug.
| eta = compute_effsize(xs, y=0, eftype="eta_square") | ||
| assert 0 <= eta <= 1 | ||
| # other types must match convert_effsize applied to the one-sample d | ||
| for eftype in ["hedges", "odds_ratio", "eta_square"]: |
There was a problem hiding this comment.
Could this loop also cover the eftype values where convert_effsize and _check_eftype disagree? cles, cohen_dz and r are the ones that regress (see the comments on effsize.py), and pointbiserialr is the one whose value depends on n:
# types that convert_effsize does not handle must still return the d
for eftype in ["cohen", "cles", "cohen_dz", "r"]:
assert np.isclose(compute_effsize(xs, y=0, eftype=eftype), d)
# pointbiserialr must not depend on the sample size
long_xs = xs * 100
assert np.isclose(
compute_effsize(xs, y=0, eftype="pointbiserialr"),
compute_effsize(long_xs, y=0, eftype="pointbiserialr"),
)The second assertion holds only once a = 4 is used, so it pins the fix down.
2 participants
For a one-sample test (scalar
y),compute_effsizeignores theeftypeargument and always returns the raw Cohen's d. In particulareftype="eta_square"returns a value greater than 1, which is impossible since eta-squared is bounded in[0, 1].The two-sample branch already converts Cohen's d to the requested effect size via
convert_effsize; the one-sample branch returnedddirectly and skipped that step. This routes the one-sample d through the sameconvert_effsizecall, soeftypeis honored: eta_square 0.462, hedges 1.610, odds_ratio 28.75, andcohenis unchanged. Added a regression test. Happy to add a changelog entry if you'd like.