Skip to content

Conversation

@JacobHass8
Copy link

This fixes the bug documented in issue scipy/#21370. For $\lambda < 0$ and large $x$, the stats.tukeylambda.cdf function would not return 0. By reducing the parameter tukey_EPS, this can be achieved.

@mdhaber
Copy link
Contributor

mdhaber commented Dec 8, 2025

Thanks. Based on the original report in gh-21370, please add a test to this class:
https://github.com/scipy/scipy/blob/b1296b9b4393e251511fe8fdd3e58c22a1124899/scipy/stats/tests/test_distributions.py#L8371
and confirm that it fails before the fix and passes after.

Maybe name it test_gh21370_cdf_sf_saturation and as the first line, include a comment summarizing the issue and what the test checks. I don't foresee a need for parameterize; you can use an array for x (and lam if necessary), and tests of the CDF and SF can go on separate lines.

@JacobHass8
Copy link
Author

Thanks. Based on the original report in gh-21370, please add a test to this class: https://github.com/scipy/scipy/blob/b1296b9b4393e251511fe8fdd3e58c22a1124899/scipy/stats/tests/test_distributions.py#L8371 and confirm that it fails before the fix and passes after.

Will do! Should I submit another PR in the scipy repository with this fix? I'm a little confused about how to go about this when xsf is a separate repository.

@mdhaber
Copy link
Contributor

mdhaber commented Dec 9, 2025

Oh, I didn't notice this was in xsf and not SciPy. @steppi can guide you on this one. After it's merged, I think you'll want to open a PR in SciPy that updates the xsf submodule commit and adds the test.

@JacobHass8
Copy link
Author

Oh, I didn't notice this was in xsf and not SciPy. @steppi can guide you on this one. After it's merged, I think you'll want to open a PR in SciPy that updates the xsf submodule commit and adds the test.

I've added a PR in scipy that tests this case (scipy/scipy/#24167). Right now it will fail, but after this PR it will pass.

@steppi
Copy link
Collaborator

steppi commented Dec 16, 2025

I think I need to take a closer look at this. I'm not confident at a glance that changing the value of tukey_EPS won't have other consequences. Also, @dschmitz89, is this something that SciPy could use Boost for instead?

@WarrenWeckesser
Copy link
Member

I worked on tukeylambda last year, but it turned into a bit of a can of worms. I'll get back to it, and continue the discussion over in scipy/scipy#21370.

@JacobHass8
Copy link
Author

I think I need to take a closer look at this. I'm not confident at a glance that changing the value of tukey_EPS won't have other consequences.

Certainly this would slow down convergence. I've seen it can take ~10 more iterations to converge to a solution with the new tukey_EPS value. I don't think it would never converge though. All the tests in scipy.stats still passed when I ran them.

@dschmitz89
Copy link
Contributor

I think I need to take a closer look at this. I'm not confident at a glance that changing the value of tukey_EPS won't have other consequences. Also, @dschmitz89, is this something that SciPy could use Boost for instead?

I am afraid boost does not include the Tukey-Lambda distribution.

@JacobHass8
Copy link
Author

I think I need to take a closer look at this. I'm not confident at a glance that changing the value of tukey_EPS won't have other consequences.

Would allowing tukey_EPS to be smaller in the tails be a good compromise as in

if (x > 1e9){
    detail::tukey_EPS = 1.0e-18;
}

There will certainly be a change in the cdf value about $x=10^9$, but surely it won't be that noticeable. I picked this value arbitrarily so it can certainly be changed.

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.

5 participants