Change skew normal quantile to use bracket_and_solve_root. #1123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rather than Newton iterations. Add test case. Fixes #1120
NAThompson
reviewed
Apr 21, 2024
test/git_issue_1120.cpp
Outdated
| total_iter_count += global_iter_count; | ||
| CHECK_LE(global_iter_count, static_cast<std::uintmax_t>(55)); | ||
| double p = cdf(dist, x); | ||
| CHECK_ULP_CLOSE(p, pn[i], 45000); |
There was a problem hiding this comment.
Given that we know that derivative of the CDF, I wonder if we could make this a bit more simple using the condition number of rootfinding, e.g.,
CHECK_ABSOLUTE_CLOSE(p, pn[i], 1/abs(pdf(dist, x));
|
@NAThompson : I tried using check_conditioned_error which seems like the right test(?) but am getting screwy results, consider: And the quantile at p = 0.0001. bracket_and_solve_root is returning at interval with just 5 fp values in it, and we can test all of them like this: But this yields apparently large errors: What am I doing wrong? |
|
Never mind... looking at the difference (In ULP) between successive cdf values I see: The next successive diffs are all zero as well, so there's a step change somewhere in the owen's t probably. |
Ah my bad, I'd missed (forgotten) that the skew normal cdf was the difference between two values. Sorry for the noise! |
|
Anyone any ideas why clang would have much higher error rates - as in nearly 50eps rather than 5? Same std lib and platform etc. |
|
OK, merging develop has made things even worse:
|
Last week I noticed this across the board on boost: boostorg/boost-ci#230. This PR contained the recommended solution: #1124. That ran fine, and same with multi-precision earlier today. Now it looks like a Microsoft issue: https://github.com/boostorg/math/actions/runs/8813624024/job/24197526207?pr=1123#step:7:86
Homebrew added clang-18 yesterday. I see some of the same problems as https://github.com/boostorg/math/actions/runs/8813624023/job/24191756783#step:11:941 locally: We also run against macOS latest so maybe they made the switch from x86_64 to ARM? Looks like it should have hit a while ago though: actions/runner-images#9254 |
|
Everything running on ubuntu 22.04 is currently broken: https://github.com/orgs/community/discussions/120966 |
|
Thanks @mborland , looks like the MS issue is now fixed. With regard to clang/MacOS, I wonder if you could run this instrumented version locally and see if shows up where things go wrong? Output should be: Thanks! |
|
Results with Homebrew Clang 18.1.4 with target arm64-apple-darwin23.4.0: Apple Clang 15.0.0 on same platform GCC 13.2.0 on same platform and GCC 11.4.0 same platform After running GCC-13 I wondered if the new support for excess precision was making a difference, but clearly not since the result with GCC-11 is the same (and there shouldn't be since Mac ARM does not support 128-bit long double like linux ARM servers do). |
|
Thanks Matt! That narrows it down, I need to be going out now, but I'll do another reduced case tomorrow, it looks though, like ellint_rf is going bad - which would be strange given that it's all basic arithmetic in there. |
|
For what it's worth @ckormanyos and I tried adding |
Maybe, it's a weird one this: I ran a fairly intensive test run comparing double to multiprecision results and could find no appreciable errors (MSVC). However, there are some definite places where cancellation occurs - at least in the Heuman Lambda case - and running some interval arithmetic tests suggests the errors should be quite large - of the order 10^5 eps. So it's possible that we're "passing by accident" at present, although that seems rather strange, and of course interval arithmetic is well known for producing over-large intervals :( What I'll do is prepare a separate PR that addresses the cancellation issues I can see, and hope that fixes the clang failures for this test: there are still several other tests failing, and the number increases in C++20 for some strange reason. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1123 +/- ##
========================================
Coverage 93.69% 93.69%
========================================
Files 770 771 +1
Lines 61070 61103 +33
========================================
+ Hits 57221 57252 +31
- Misses 3849 3851 +2
Continue to review full report in Codecov by Sentry.
|
|
Thanks for the quick fix @jzmaddock, much appreciated. SciPy PR should land soon: scipy/scipy#20643 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rather than Newton iterations.
Add test case.
Fixes #1120