Skip to content

Fix srdist by passing nranges=1 to Rmath.ptukey/qtukey - #211

Merged
andreasnoack merged 1 commit into
masterfrom
dmw/fix-srdist
Jun 3, 2026
Merged

andreasnoack merged 1 commit into
masterfrom
dmw/fix-srdist

Conversation

@devmotion

Copy link
Copy Markdown
Member

Summary

The inlining of the RFunctions macro in #188 dropped the special-case that
appended a literal 1 for :tukey, so the lower_tail/log_p Bool flags
shifted into the nranges positional slot of Rmath.ptukey/qtukey. As a
result, srdistcdf ↔ srdistccdf are silently inverted on v2.0.0 and the
inverse CDF functions also return wrong values. For example:

julia> using StatsFuns

julia> srdistcdf(2.0, 2.0, 0.0)         # should be 0
1.0

Restore the missing 1 argument explicitly in all 8 Rmath.ptukey/qtukey
calls.

Why the existing tests didn't catch it

The rmathcomp harness compared StatsFuns' buggy srdistcdf against an
equally buggy direct Rmath.ptukey(..., true, false) call, so the test
agreed with itself. Fix the harness to inject nranges=1 for tukey when
building the Rmath-side lambdas, and add a strict-equality
@testset "srdist boundary behaviour" covering x = 0 and x = Inf for
all four cdf-like functions.

Test plan

  • Pkg.test() passes locally (full suite)
  • New srdist boundary behaviour testset passes (8 strict == assertions)
  • Verified Distributions.jl's StudentizedRange and
    Truncated{StudentizedRange} test failures on Update to StatsFuns@2 Distributions.jl#2062
    go away when this branch is dev'd in

🤖 Generated with Claude Code

The inlining of the `RFunctions` macro in #188 dropped the special-case that
appended a literal `1` for `:tukey`, so the `lower_tail`/`log_p` Bool flags
shifted into the `nranges` positional slot and inverted CDF↔CCDF (e.g.
`srdistcdf(2, 2, 0) == 1.0` instead of `0`). Restore the argument explicitly.

The existing rmathcomp tests compared StatsFuns' buggy `srdistcdf` against an
equally buggy direct `Rmath.ptukey(..., true, false)` call, so the regression
went unnoticed; fix the test harness to inject `nranges=1` for tukey and add
a strict boundary-behaviour testset covering `x = 0` and `x = Inf`.
@devmotion
devmotion marked this pull request as ready for review May 28, 2026 18:29
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.39%. Comparing base (53dc188) to head (72e46fe).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #211   +/-   ##
=======================================
  Coverage   72.39%   72.39%           
=======================================
  Files          22       22           
  Lines        1007     1007           
=======================================
  Hits          729      729           
  Misses        278      278           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andreasnoack
andreasnoack merged commit 21903f7 into master Jun 3, 2026
12 checks passed
@andreasnoack
andreasnoack deleted the dmw/fix-srdist branch June 3, 2026 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants