aesara-devs / aehmc

An HMC/NUTS implementation in Aesara
MIT License
33 stars 6 forks source link

Return inner-`Scan` updates instead of setting `default_update`s #55

Closed brandonwillard closed 2 years ago

brandonwillard commented 2 years ago

This PR replaces our use of .default_updates with the existing inner-function return value semantics, which is capable of handling updates. These changes fixes some missing input issues caused by the hand-crafted .default_updates.

brandonwillard commented 2 years ago

The current error is:

FAILED tests/test_hmc.py::test_nuts - assert 1.2154702717073333 == 1.0 ± 1.0e-01

@rlouf, do you think this is within an acceptable range, or a sign of something being off?

codecov[bot] commented 2 years ago

Codecov Report

Merging #55 (d1a7d8e) into main (9f982cc) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #55   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          464       461    -3     
  Branches        26        24    -2     
=========================================
- Hits           464       461    -3     
Impacted Files Coverage Δ
aehmc/trajectory.py 100.00% <ø> (ø)
aehmc/step_size.py 100.00% <100.00%> (ø)
aehmc/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9f982cc...d1a7d8e. Read the comment docs.

rlouf commented 2 years ago

Great, this makes the code cleaner and easier to understand.

@rlouf, do you think this is within an acceptable range, or a sign of something being off?

So close to the target it's likely to be a problem with the parameters, most likely step_size.

Edit: I see the tests pass with a smaller step size. No reason to be concerned then.