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

Fix inverse_discrete_quantile for large guess #1007

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

Yuhta
Copy link
Contributor

@Yuhta Yuhta commented Jul 31, 2023

If guess passed to inverse_discrete_quantile cannot be represented as floating point number, it is possible that guess + 1 or guess - 1 does not change the value at all and we are stuck in infinite loop inside round_to_floor or round_to_ceil. Fix this by increase/decrease more than 1 in these cases.

Example code to reproduce this:

boost::math::binomial_distribution<> dist(9079765771874083840, 0.561815);
boost::math::quantile(dist, 0.0365346);

@Yuhta Yuhta force-pushed the fix-inverse_discrete_quantile branch from 320db24 to 071d338 Compare July 31, 2023 03:24
test/test_binomial.cpp Outdated Show resolved Hide resolved
@Yuhta Yuhta marked this pull request as draft July 31, 2023 15:29
@Yuhta Yuhta force-pushed the fix-inverse_discrete_quantile branch 3 times, most recently from d20c75c to aab8163 Compare August 1, 2023 00:51
@Yuhta Yuhta marked this pull request as ready for review August 1, 2023 01:27
@Yuhta Yuhta requested a review from mborland August 1, 2023 01:27
Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

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

Outside of the overflow this makes sense to me. We have been dealing with non-representable numbers in other places recently too: #968

binomial_distribution<RealType, Policy> dist(9079765771874083840, 0.561815);
// Accuracy is not too important here; the main purpose is to make
// sure it is not stuck.
BOOST_CHECK_CLOSE(quantile(dist, 0.0365346), 5101148604445670400, 1e12);
Copy link
Member

Choose a reason for hiding this comment

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

On MSVC platforms you are getting an overflow here: https://github.com/boostorg/math/actions/runs/5721216709/job/15503232562#step:11:2377. boost::math::concepts::real_concept is a long double wrapped in a class to check conceptual requirements for support of user defined types.

Copy link
Contributor Author

@Yuhta Yuhta Aug 5, 2023

Choose a reason for hiding this comment

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

@mborland It's an unrelated bug in

return prefix * exp(a * log(x * c / a) + b * log(y * c / b)) * scaled_tgamma_no_lanczos(c, pol) / (scaled_tgamma_no_lanczos(a, pol) * scaled_tgamma_no_lanczos(b, pol));
where a * log(x * c / a) + b * log(y * c / b)) is less accurate on MSVC because long double is just double there.

Can I skip real_concept test on MSVC for this test case?

Copy link
Member

@mborland mborland Aug 7, 2023

Choose a reason for hiding this comment

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

I ran your branch locally on an M1 Mac (double = long double) and that works fine. I should have time this week to investigate why MSVC wants to be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mborland Note that in the newest version I skip the test for real_concept if long double is not precise enough. I can reproduce the overflow on Linux if I force real_concept_base_type to be double.

@Yuhta Yuhta marked this pull request as draft August 3, 2023 02:30
@Yuhta Yuhta force-pushed the fix-inverse_discrete_quantile branch from 6bcde0a to e09f377 Compare August 5, 2023 03:25
If `guess` passed to `inverse_discrete_quantile` cannot be represented
as floating point number, it is possible that `guess + 1` or `guess - 1`
does not change the value at all and we are stuck in infinite loop
inside `round_to_floor` or `round_to_ceil`.  Fix this by
increase/decrease more than 1 in these cases.

Example code to reproduce this:
```c++
boost::math::binomial_distribution<> dist(9079765771874083840, 0.561815);
boost::math::quantile(dist, 0.0365346);
```
@Yuhta Yuhta force-pushed the fix-inverse_discrete_quantile branch from e09f377 to ee80892 Compare August 5, 2023 16:53
@Yuhta Yuhta marked this pull request as ready for review August 5, 2023 17:15
@Yuhta Yuhta requested a review from mborland August 5, 2023 17:15
@ethanyzhang
Copy link

@mborland Gentle ping :)

Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

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

Sorry about having this sit. Once the CI cycles I can merge it in. Beta is having other overflows documented here: #1009. Like your test case it takes some extreme inputs to hit these so I am not overly concerned.

@@ -716,6 +716,18 @@ void test_spots(RealType T)

check_out_of_range<boost::math::binomial_distribution<RealType> >(1, 1); // (All) valid constructor parameter values.

// TODO: Generic ibeta_power_terms has accuracy issue when long
// double is not precise enough, causing overflow in this case.
if(!std::is_same<RealType, real_concept>::value ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens here when T is a double, does that still work? Ideally we shouldn't be having spurious internal overflow as a) it breaks types with no infinity (this is potentially true for long double too) and b) it sets the hardware overflow flag which can trip up downstream consumers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is in beta.hpp#478 onwards. Don't see a trivial fix just yet :(

inv_distrete_quantile.hpp#150 also needs adjusting to else if((adder != 0) && (a + adder != a)) to speed up this new test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With built-in double and long double it works fine because Lanczos approximation is enabled.

@Yuhta
Copy link
Contributor Author

Yuhta commented Aug 19, 2023

@mborland It seems one CI job has environmental issue: https://drone.cpp.al/boostorg/math/1435/242/1
Can you rerun it or I need to push again?

@mborland
Copy link
Member

@mborland It seems one CI job has environmental issue: https://drone.cpp.al/boostorg/math/1435/242/1

Can you rerun it or I need to push again?

I restarted it. The s390x runners can be finicky.

@Yuhta
Copy link
Contributor Author

Yuhta commented Aug 20, 2023

@mborland It fails with similar thing but on a different job: https://drone.cpp.al/boostorg/math/1450/66/1

@mborland mborland merged commit 74bb4bc into boostorg:develop Aug 24, 2023
55 checks passed
jzmaddock added a commit that referenced this pull request Aug 30, 2023
…the non Lanczos case).

Completes work started here: #1007
czentgr added a commit to czentgr/velox that referenced this pull request Feb 6, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 6, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 6, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 6, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 8, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 8, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 9, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 14, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 19, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 26, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
czentgr added a commit to czentgr/velox that referenced this pull request Feb 29, 2024
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
facebook-github-bot pushed a commit to facebookincubator/velox that referenced this pull request Mar 1, 2024
Summary:
The boost version 1.84.0 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The update occurs for the setup scripts for CentOS and Ubuntu. MacOS already installs the
latest available Boost version on brew.

In addition, the requirement for the Boost version is updated to make sure the proper version is used.
There is no known build problem when upgrading the version.
Fixes issue: #8624

Pull Request resolved: #8679

Reviewed By: Yuhta

Differential Revision: D54384316

Pulled By: kgpai

fbshipit-source-id: 7442b028a9b2f1b36b3190a7909a4a55ef52e72c
@jzmaddock
Copy link
Collaborator

@Yuhta : sorry to revisit this... but this, or at least this new test case is causing failures on MacOS/Clang, and after some investigation, it seems that Clang is not so much wrong to fail, as other compilers are lucky to pass.

The issue is this: the binomial distribution for large parameters calls the incomplete Beta, and the incomplete Beta for parameters greater than about 1/epsilon essentially returns garbage. There is no way around this that I can see - we go through all sorts sorts of contortions to try and avoid using logarithmic calculations and to preserve digits when we do - but in this case all the digits in the log calculation cancel out and we're left not even really knowing the order of magnitude of the result. All the alternate methods of calculation I tried are essentially worse, with interval arithmetic yielding [0, +INF] as the bounds on the result. Ouch.

So... what was the use case here? And what would happen if instead returning garbage we just gave up and threw an evaluation_error for large parameters? I'm not saying that that is the correct way to go, just can't think of anything better right now...

See also discussion inhttps://github.com//pull/1125.

CC @mborland

@Yuhta
Copy link
Contributor Author

Yuhta commented Apr 29, 2024

@jzmaddock We are ok with evaluation_error, before my fix it was stuck there forever, we just wanted to avoid that situation, the result in this case is not usable anyway.

@jzmaddock
Copy link
Collaborator

Thanks @Yuhta , I've decided to just disable the test for apple/clang for now - it does return, but returns garbage.

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.

4 participants