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

MAINT: stats: Unconditionally disable boost double promotion. #17482

Merged
merged 2 commits into from Nov 27, 2022

Conversation

WarrenWeckesser
Copy link
Member

@WarrenWeckesser WarrenWeckesser commented Nov 25, 2022

Set the Boost double promotion policy to false on all platforms.

@WarrenWeckesser WarrenWeckesser marked this pull request as ready for review November 26, 2022 19:57
@WarrenWeckesser WarrenWeckesser added scipy.stats C/C++ Items related to the internal C/C++ code base labels Nov 26, 2022
@WarrenWeckesser
Copy link
Member Author

This pull request continues a conversation from back when @mckib2 added the wrappers for the boost distributions to stats. The boost code has the option to automatically promote double to long double for internal calculations (trading off performance for higher precision). In the original pull request, this option was enabled for 32 bit platforms, but not 64 bit platforms.

I think it would be better to be consistent in how we apply the promotion rule across all platforms. It shouldn't be necessary to change how double precision floats are handled based on whether the platform is 32 or 64 bits. In this pull request, I have disabled the promotion unconditionally. Only one test had to be fixed. That test computes a round-trip through the cdf and ppf functions of a discrete distribution. That test was already "dangerous", in that it was computing the quantile function for a floating point input that is exactly at a discontinuity of the quantile function [1]. That is, if the value of the CDF is increased by one ULP, the correct answer jumps to the next higher integer. To make the test more robust, I modified it so that the computed CDF values are decreased by 10 ULPs before passing them to the quantile function.


[1] Here's an example of the sensitivity of this calculation. nbinom.ppf(nbinom.cdf(6, 5, 0.5), 5, 0.5) returns 6, and nbinom.ppf(np.nextafter(nbinom.cdf(6, 5, 0.5), 1), 5, 0.5) returns 7, as desired. But consider values just slightly less than nbinom.cdf(6, 5, 0.5) passed to nbinom.ppf:

>>> c = nbinom.cdf(6, 5, 0.5)
>>> cvals = c - np.spacing(c)*np.arange(10)
>>> nbinom.ppf(cvals, 5, 0.5)  # All values should be 6.
array([6., 6., 7., 6., 6., 6., 6., 7., 6., 6.])

Because of this sensitivity, we can't rely on the round-trip calculation always giving the correct result.

@mckib2
Copy link
Contributor

mckib2 commented Nov 27, 2022

The history of why this was enabled I recall had to do with certain tests almost always on 32-bit runners failing because of the lack of precision, e.g., gh-16591

@WarrenWeckesser Fantastic! This has been on the list of things to get done and is a good move for consistency. LGTM and all tests are green, so will merge

@mckib2 mckib2 merged commit dd17742 into scipy:main Nov 27, 2022
@WarrenWeckesser WarrenWeckesser deleted the boost-stats-no-double-promote branch November 27, 2022 04:32
@WarrenWeckesser WarrenWeckesser added this to the 1.10.0 milestone Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C++ Items related to the internal C/C++ code base scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants