fix: invert the one_of quantile by bisection, not Nelder-Mead (#356) - #376
Closed
seabbs-bot wants to merge 2 commits into
Closed
fix: invert the one_of quantile by bisection, not Nelder-Mead (#356)#376seabbs-bot wants to merge 2 commits into
seabbs-bot wants to merge 2 commits into
Conversation
The Windows Julia 1.x Test cell hung to the 60-minute job timeout. The job log's last line is the ComposedDistributionsOptimizationExt precompile, then 55 minutes of silence. That precompile is triggered by the first quantile testitem's `using Optimization, OptimizationOptimJL`, and the first thing it then does is a Nelder-Mead solve. Windows on the Julia pre release runs the same testitems, against the same Optim 2.2.1 / Optimization 5.7.0 / SciMLBase 3.44.0 resolution, in under 8 minutes. The #358 maxtime bound could not have helped. It reaches Optim's `time_limit`, which is checked between iterations, so it cannot interrupt a single evaluation. A cdf is monotone, so its inverse is a root find, not an optimisation. Resolve and Compete now bisect their own cdf over a bracket grown from the support by doubling, which terminates in a step count fixed by construction. The bracket needs no mean, so the non-finite starting point of #344 is gone too. Measured against the old solve over five node shapes at ten probabilities each, the worst |cdf(quantile(d, p)) - p| falls from 3.7e-5 to 2.2e-16, and a call is roughly nine times faster. No solver is involved any more, so the methods move into src and the Optimization / OptimizationOptimJL extension goes away. The quantile now answers with nothing but the package loaded, and a subprocess testitem pins that. A Sequential chain's quantile still delegates to ConvolvedDistributions' Convolved, whose own quantile is the same Nelder-Mead solve, so the two testitems covering that arm still load the solver stack. The same bracketed root find belongs upstream. Also make a future hang legible. TestItemRunner prints nothing until its summary, so runtests.jl now runs one test file at a time, announcing the file and its items before starting and the wall clock after. A hang names the file it is in instead of dying silently at the job timeout.
Contributor
Try this Pull Request!Option 1: Julia Package ManagerOpen Julia and type: import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/EpiAware/ComposedDistributions.jl", rev="fix/356-quantile-root-find")
using ComposedDistributionsOption 2: Local CheckoutIf you have the repo locally: git checkout fix/356-quantile-root-find
julia --project=. -e "using Pkg; Pkg.instantiate()" |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Contributor
|
📖 Documentation preview is ready! View the docs for this PR at: https://EpiAware.github.io/ComposedDistributions.jl/previews/PR376/ This preview will be updated automatically when you push new commits. |
seabbs
reviewed
Aug 12, 2026
seabbs
left a comment
seabbs
left a comment
Member
There was a problem hiding this comment.
I'm really not convinced we want our hand rolled method can't we just see if the issue has cleared and or use a different more approriate sovler or other?
Contributor
Benchmark comparison vs baseMinimum time per call. Buckets are PR time as a % of base, so lower is faster (🟢 faster, ⚪ within 5%, 🔴 slower). Counts of benchmarks per bucket:
Evaluation — 17 benchmarks (by time change)
AD gradients — 36 benchmarks (by time change)
|
Collaborator
Author
|
Superseded by #381, which folds this and seven other PRs into one branch off This branch is preserved and the PR body is kept for its rationale — the detail here is the record of why, which #381 summarises rather than repeats. Closing so it does not compete for review. This comment was posted by a bot. Please ping @seabbs for any questions. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes the Windows Julia 1.x hang by removing the failure mode rather than bounding it. Also deletes an extension and two weak dependencies.
Stacked on #359 — the diff shown against
mainincludes that PR's compat fix, because main cannot instantiate without it. Merge #359 first; this is one commit on top.Diagnosis, narrowed
The hanging cell's log runs to 17:34:06 and then goes silent to the 60-minute timeout. The last two precompiles are the two Optimization extensions, both triggered by the single
using Optimization, OptimizationOptimJLintest/composers/quantile_optimization.jl. Testitems run in file order, so the trigger is that file's first item, whose very next act is the firstOptimization.solve.Resolved versions are byte-identical between the hanging Windows cell and the passing Windows
precell — Optim 2.2.1, Optimization 5.7.0, OptimizationOptimJL 0.4.18, SciMLBase 3.44.0. So this is Julia 1.12.6 on Windows against that solver stack, not a resolution difference.The hang could not be reproduced locally on the same Julia and the same stack. So this is a well-evidenced hypothesis, not a proven cure, and Windows CI on this branch is the real test.
The fix
A CDF is monotone, so inverting it with a derivative-free simplex method was the wrong tool. Bisection has a bounded iteration count by construction, which removes the failure mode instead of putting a wall-clock bound near it — the bound #358 added could never have worked, since Optim only checks
time_limitbetween iterations and cannot interrupt a singlecdfevaluation.New
src/composers/one_of_quantile.jl: bracket grown fromminimum/maximumand doubled outward when an end is unbounded (capped at 200 steps), bisection capped at 100 halvings, exiting when the midpoint lands on adjacent floats. Boundary shortcuts forp = 0/p = 1and forpoutside the reachable cdf of a defective race.It never calls
mean(d), so #344's non-finite starting point cannot recur, and aCompetequantile no longer pays for a panelled-quadrature mean.Accuracy and speed
Five node shapes at ten probabilities each, comparing
|cdf(quantile(d, p)) - p|:Round-trip tolerances tightened from
atol = 1e-3toatol = 1e-10, and checked atrtol = 1e-8against an independently written bisection reference.Breaking, and an improvement
ext/ComposedDistributionsOptimizationExt.jlis deleted, along with theOptimization/OptimizationOptimJLweak dependencies, the extension entry and its compat. No solver is involved any more, so the methods move intosrc.quantileonResolve/Competenow works with nothing but this package loaded. A subprocess testitem pins that. Anyone who was loading Optimization solely to get these quantiles no longer needs to.Legibility, so a future hang names itself
test/runtests.jlruns one file at a time, printing a timestamped banner naming the file and its testitems before, and the wall clock after, with explicit flushing — so a wedged process leaves its last banner in the log. Per-testitem granularity is not reachable without reimplementing TestItemRunner's run loop, since itsfilterhook fires only at discovery; file-level plus the item list is the honest ceiling. Cost is about 0.4 s per file.It has already earned its keep: a local run showed one file taking 955 s against 11.4 s clean, which turned out to be contention from another process on the same box. Previously that would have been invisible.
The CD-side fix may not be sufficient alone
quantile_by_optimizationstill lives in ConvolvedDistributions and still backsConvolved/Difference/Product/Ratio. This package still reaches it two ways — a multi-stepSequentialdelegates to itsConvolvedtotal, andCompete's panel-break ladder callsquantileon a composite cause — so two testitems intest/composers/quantile.jlstill load the solver stack. Tracked as an upstream change on our own repo. If Windows still hangs, the new banners will name the file, and the candidates are down to those two.Gates
task test-fastpass across all 22 files,task test-quality239 pass,task docs-fastbuilds with all new@refs resolving. Quantile paths exercised directly forResolveandCompeteincluding Cauchy (no finite mean), Pareto (unbounded mean — #344's cases), bounded support, a defective race, and monotonicity acrossp.Closes #356. Refs #344.
This was opened by a bot. Please ping @seabbs for any questions.