Quantile: use Newton method from Roots.jl
#1900
Open
Conversation
This file contains hidden or 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
|
Notably |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1900 +/- ##
==========================================
- Coverage 86.28% 86.24% -0.05%
==========================================
Files 146 146
Lines 8787 8775 -12
==========================================
- Hits 7582 7568 -14
- Misses 1205 1207 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
And |
|
The problem is (jl_PRakDJ) pkg> why Test
Roots → Accessors → Test
(jl_PRakDJ) pkg> why ChainRulesCore
Roots → ChainRulesCore |
|
I made a PR to Roots that would fix the ChainRulesCore issue: JuliaMath/Roots.jl#445 |
|
Awesome! Now they need to do a release :] |
LebedevRI
force-pushed
the
roots.jl
branch
4 times, most recently
from
5a13d02 to
4fe57eb
Compare
LebedevRI
added a commit
to LebedevRI/Distributions.jl
that referenced
this pull request
Sep 13, 2024
In JuliaStats#1900, dependency on Roots.jl is being added, which only supports julia 1.6+
|
Next issue: Distributions.jl/test/multivariate/dirichlet.jl Lines 137 to 142 in b219803 I suspect it was always there and just not caught by CI because only two jula versions are being tested... |
|
Looking at the wider picture,
All of these happen in the current master regardless of this PR. |
It is somewhat expected that this highly depends on the Julia versions as some of the ambiguities might disappear due to upstream changes which might not be available in older Julia or package versions (the latest package versions might not be installable on older Julia versions). The last time I checked ambiguity issues were caused by upstream packages, so there's nothing we can do about it (and if users are actually affected by them, they can always update to a newer Julia versions).
I assume the 1.8 errors are also caused by AD tests? I think there's nothing to be worried about these (apart from that it causes CI failures) since such errors often show up when performing AD tests close to the boundary of the domain (perturbations might lead to e.g. negative values or negative values of which we compute If these are the only test failures on older Julia versions, I actually think there are no major problems with these Julia versions. |
I can't tell:
Yes, but what about the CI? Should CI test j1.7 instead of (failing) j1.6? |
|
I've looked, and this randomness randomness is a rather known issue: All in all, i've spent more time than i would have wanted, |
|
I'm not sure if this is waiting on something from my side, in which case that needs to be communicated more clearly. |
|
ping |
This is what the `quantile_newton()`/`cquantile_newton()` does, because otherwise they were able to end up in an endless loops, when the initial point and the mode are the same, see JuliaStats#666. I'm not sure this is needed here, but the next change is going to refactor them to use general `newton()`, which would make this change anyway, so unless we need the current behaviour, let's do this change explicitly.
… the `newton() These have `df(x)=1`, at least that is what the `df(x)` is if we solve `Δ(x)=f(x)/f'(x)` as an equation for `f'(x)`.
Note that we really do need `Roots.jl` 2.2.0-or-newer, because of JuliaMath/Roots.jl#445
|
@devmotion Hi! Now that julia-1.10 is required anyways, i believe this PR became trivial. |
LebedevRI
commented
May 24, 2025
| function quantile_newton(d::ContinuousUnivariateDistribution, p::Real, xs::Real=mode(d), tol::Real=1e-12) | ||
| x = xs + (p - cdf(d, xs)) / pdf(d, xs) | ||
| function newton((f,df), xs::T=mode(d), xrtol::Real=1e-12) where {T} | ||
| return find_zero((f,df), xs, Roots.Newton(), xatol=0, xrtol=xrtol, atol=0, rtol=eps(float(T)), maxiters=typemax(Int)) |
There was a problem hiding this comment.
Notably, it would be sufficient to pass a reasonable maxiters to fix the currently-present bug.
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.
In #1899,
i'm trying to address the oscillation problem of Newton method,
which does happen in real-world, non-synthetic distributions.
As per #1899 (comment),
let's first stop reinventing the wheel.