Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Correct intermediate overflow in KS one asymptotic in SciPy.stats #17783

Merged
merged 1 commit into from Jan 13, 2023

Conversation

aherbert
Copy link
Contributor

Overflow can occur for 6*n when integer n is large.

Change the use of exp to expm1 when appropriate.

See #17775

Reference issue

Closes gh-17775.

What does this implement/fix?

Corrects integer overflow in the c implementation when n is large by changing 6*n to 6.0*n.

Use exp or expm1 to compute the p-value choosing whichever will be smallest. The opposite p-value is computed by subtraction from 1.

@tupui tupui added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.stats labels Jan 13, 2023
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @aherbert, this looks great. I only have some suggestions.

Also linter is failing. You can check locally doing python dev.py lint.

scipy/stats/tests/test_continuous_basic.py Outdated Show resolved Hide resolved
scipy/stats/tests/test_continuous_basic.py Outdated Show resolved Hide resolved
@tupui tupui added this to the 1.11.0 milestone Jan 13, 2023
Overflow can occur for 6*n when integer n is large.

Change the use of exp to expm1 when appropriate.

See scipy#17775
@aherbert
Copy link
Contributor Author

assert_allclose uses a tolerance relative to the expected. This removed the requirement to use the ulp assertion. I switched to a parameterized test which makes it easier to add more test cases as required. Currently the 3 provided cases all failed with the old C code.

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's see if all CI is happy now. Thanks for the quick updates.

@tupui
Copy link
Member

tupui commented Jan 13, 2023

CI failure is not related (and now fixed on main). Merging. Thanks again @aherbert, it was a great issue and PR 👍

@tupui tupui merged commit 841acde into scipy:main Jan 13, 2023
@tupui
Copy link
Member

tupui commented Jan 13, 2023

Oh just for next time, try to avoid force pushing without telling first the reviewer. GitHub is not smart enough and links from email notifications are breaking after a force push. But even if, it's simpler for reviewer to know what commit they already reviewed and follow along.

@aherbert
Copy link
Contributor Author

OK. Next time I will leave the commit history and you can squash them as appropriate.

@tupui
Copy link
Member

tupui commented Jan 13, 2023

Yes this is what we do 😃

@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Feb 18, 2023
@tylerjereddy
Copy link
Contributor

Bug fix shortly after 1.10.0 release so tentatively labeling for backport to 1.10.1

@tylerjereddy tylerjereddy modified the milestones: 1.11.0, 1.10.1 Feb 18, 2023
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request Feb 18, 2023
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Asymptotic computation of ksone.sf has intermediate overflow
3 participants