Issue 340: Remove nan handling #415
Merged
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
Try this Pull Request!Open Julia and type: import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/CDCgov/Rt-without-renewal", rev="remove-nan-handling", subdir="EpiAware")
using EpiAware |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
- Coverage 93.57% 88.67% -4.91%
==========================================
Files 54 56 +2
Lines 560 715 +155
==========================================
+ Hits 524 634 +110
- Misses 36 81 +45 ☔ View full report in Codecov by Sentry. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
|
So the overflow tests semi-obviously fail now but waiting on the benchmarks. The documenter issue appears to be unrelated but unclear |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
|
So no warnings in the benchmarks about gradient issues and some evidence of a speed up for NB error models. Not clear if this is worth it if we then have overflow issues? Personally I am minded to remove and see how we go? |
seabbs
force-pushed
the
remove-nan-handling
branch
from
7c8b12f
to
d3caa10
Compare
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
The main problem here is that an overflow error it can be irritating inside of an inference/NUTS run. Obviously, we can put a A feature of this issue/PR is that the using Distributions, ReverseDiff, FiniteDifferences
function NegativeBinomialMeanClust(μ, α)
μ² = μ^2
ex_σ² = α * μ²
p = μ / (μ + ex_σ²)
r = μ² / ex_σ²
return NegativeBinomial(r, p)
end
log_μ = 48. #Plausible large value to hit with a log scale random walk over a number of time steps
α = 0.1
nb = NegativeBinomialMeanClust(exp(log_μ), α)
#A few checks
logpdf(nb, 100)
# -427.89305594455965
# Make a helper function for grad calls
f(x) = NegativeBinomialMeanClust(exp(x[1]), α) |> nb -> logpdf(nb, 100)
# Compare finite diff and rev diff at log_mean = log_μ
g_fd = grad(central_fdm(5, 1), f, [log_μ])[1]
#Precompile ReverseDiff using arbitrary input value to compile the tape
input = randn(1)
const f_tape = ReverseDiff.GradientTape(f, input)
const compiled_f_tape = ReverseDiff.compile(f_tape)
cfg = ReverseDiff.GradientConfig(input)
g_rvd = ReverseDiff.gradient(f, [log_μ], cfg)
g_fd ≈ g_rvd
#trueThe the overflow issue seems to occur when we make a sample call to a NegativeBinomial with a very large mean (in this example we have rand(nb)
Following this stack trace, the This suggests a few different options:
|
|
I think we should make an issue upstream and if we can use a sampler that doesn't overflow (this requires us to know if its a sampling context right). |
|
Another option is to make this a true Distribution and to write our own |
|
I think this is yet another example that Turing needs a sampling context |
This was my thinking. It wouldn't be too hard... basically we just need |
|
I think Turing might be the right place to flag this, but I might lower the flag down to |
Upon reflection this is sufficiently low effort I'm going to give this a go. |
|
#418 resolves the documenter CI problem here by shifting to "safe" version of the distributions. |
#418) * SafePoisson with safety for large means * better selection for conversion to Int or BigInt * add SafeNegativeBinomial * add unit tests to doctests * reformat * Add type promotion so AD works with distribution constructor * Add logpdf grad call unit tests for Safe discrete dists * reformat * change neg bin param to (r, p) * Update utils.jl * reformat * change empirical var test to more principled approach * add default nadapts rather than just 50% of target sampling * Update NUTSampler.jl * set dist check_args = false * Set nadapts to Turing Default * reformat
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
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.
This PR explores some of the findings in #340 and #414 which indicate that NegativeBinomialError has unstable gradients when a tape is used. As the majority of the code is shared with PoissonError this indicates the problem is likely to be within the observation_error function. Looking at this branch it seems like the ifelse branch is most likely to be causing issues.
It isn't clear to me if the lack of nan handling will outweigh any improvement in gradient stability. Really opening this PR for comparative benchmarks in the first instance.
I checked out some model implementations by the Turing folks and when they have implemented different parameterizations they haven't added anything for numerical stability. I've removed all the
clampstatements to test this.I haven't been able to find anything on the stability on
clamp.