Make the HMC & NUTS kernels return scan updates
#56
Conversation
|
Despite the seemingly minimal change, |
rlouf
force-pushed
the
return-internal-updates
branch
from
33addff to
e006a4e
Compare
rlouf
force-pushed
the
return-internal-updates
branch
from
e006a4e to
84c4ca2
Compare
There was a problem hiding this comment.
There doesn't appear to be anything wrong with the updates-related changes.
Despite the seemingly minimal change,
test_nuts_mcsedoes not pass anymore which is an indication that something is seriously wrong.
I can reproduce the CI error in test_nuts, but test_nuts_mcse passes locally. Changing the RNG seed in test_nuts affects the pass/fail results significantly, yet the values are all within at least a +-1 range of the "true" values.
It's not clear to me why those step size, mass matrix values, etc., should guarantee errors within the test ranges, so there's not much more I can say than that the tests look flaky.
Also, the function evaluation time seem rather long for these toy problems; are there any tests like these that check the trajectory statistics as well? If not, it looks like we need them.
|
I was thinking about getting rid of all non-mcse tests. As you said, they're flaky as a way to assess the correctness of samples and they take a long time to run. |
Codecov Report
@@ Coverage Diff @@
## main #56 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 13 13
Lines 461 461
Branches 24 24
=========================================
Hits 461 461
Continue to review full report at Codecov.
|
|
Also, we can keep the old tests, but mark them as |
rlouf
force-pushed
the
return-internal-updates
branch
2 times, most recently
from
e0c58ac to
43a052d
Compare
tests/test_hmc.py
Outdated
| return ess | ||
|
|
||
|
|
||
| def compute_mcse(samples, true_param): |
There was a problem hiding this comment.
Does this function name still apply; it doesn't appear to be returning a standard error?
tests/test_hmc.py
Outdated
| def compute_mcse(samples, true_param): | ||
| """Compute the probability of getting an error more extreme | ||
| than the one we observe assuming the samples are really drawn | ||
| from the target distirbution. |
There was a problem hiding this comment.
Is this assuming a normal target distribution?
| ], | ||
| non_sequences=0.5, | ||
| n_steps=2000, | ||
| non_sequences=1.0, |
There was a problem hiding this comment.
Does it not pass with a smaller step size?
There was a problem hiding this comment.
It does. Here I adapted the step size manually to get an average acceptance probability of 0.8 (target in the window adaptation warmup); it also runs slightly faster.
rlouf
force-pushed
the
return-internal-updates
branch
from
43a052d to
b0104d9
Compare
README.rst
Outdated
| next_step, _ = kernel(*initial_state, 1e-2) | ||
| print(next_step[0][1].eval({y_vv: 0})) |
There was a problem hiding this comment.
Our examples should probably use aesara.function and the returned updates (e.g. to avoid potentially confusing users down the line).
brandonwillard
force-pushed
the
return-internal-updates
branch
from
b0104d9 to
56ee318
Compare
brandonwillard
changed the title
scan updates
2 participants
Since #55 the inner scans do not use
default_updates but instead return the inner updates following scan's inner function semantics. In this PR we make kernel return inner updates as well.