Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sample remaining variables with the NUTS sampler #68

Merged
merged 2 commits into from
Oct 18, 2022

Conversation

rlouf
Copy link
Member

@rlouf rlouf commented Oct 3, 2022

In this PR we assign the NUTS sampler to the variables that have not been assigned a sampler in construct_sampler.

More details to come

Closes #47

@rlouf rlouf force-pushed the assign-aehmc-step branch from f0d1890 to 8ec4d98 Compare October 3, 2022 17:56
@rlouf

This comment was marked as off-topic.

@rlouf rlouf force-pushed the assign-aehmc-step branch from 8ec4d98 to 87d1aa8 Compare October 6, 2022 12:38
@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Base: 97.38% // Head: 97.41% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (3a783e4) compared to base (ed60d94).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   97.38%   97.41%   +0.02%     
==========================================
  Files           9        9              
  Lines         613      619       +6     
  Branches       60       58       -2     
==========================================
+ Hits          597      603       +6     
  Misses          5        5              
  Partials       11       11              
Impacted Files Coverage Δ
aemcmc/basic.py 100.00% <100.00%> (ø)
aemcmc/nuts.py 98.03% <100.00%> (-0.04%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rlouf rlouf force-pushed the assign-aehmc-step branch 2 times, most recently from 2cbf04f to 6d9f8c2 Compare October 6, 2022 13:45
@rlouf
Copy link
Member Author

rlouf commented Oct 6, 2022

Progress report

The NUTS sampler is integrated in construct_sampler; it uses the previous sampling_step to condition the logprob. There might be a performance trade-off with the current implementation as I re-initialize the state at every step; hopefully Aesara is able to identify the extra gradient computation is not needed, but that's something we will need to check.

It is currently tested on an example with NUTS-only. Doing so I discovered a bug when the variable is a scalar:

import aesara.tensor as at

srng = at.random.RandomStream(0)
tau_rv = srng.halfcauchy(0, 1, name="tau")
Y_rv = srng.halfcauchy(0, tau_rv, name="Y")

Which raises:

TypeError: Inconsistency in the inner graph of scan 'scan_fn' : an input and an output are associated with the same recurrent state and should have compatible types but have type 'TensorType(float64, (1,))' and 'TensorType(float64, (None,))' respectively.

This is reminiscent of shape issues I encountered in aehmc itself and I think the issue has to be solved there since it otherwise samples with no difficulty.

Most importantly, I need an example where there's both a Gibbs sampling step / closed form posterior and a NUTS sampling step.

To summarize, in order of priority:

  • Add a test with mixed samplers
  • Fix the shape issue for scalar random variables.
    This is due to RVs in Aesara having either a concrete or symbolic shape depending on the input. I opened an issue on Aesara: The shape of the RandomVariables is hard to reason about aesara#1252.
    Edit: I now specify the shape of inverse_mass_matrix when I initialize it. It doesn't solve the issue in Aesara, but this is good practice anyway and the tests pass.
  • Assess whether we occur a performance penalty by initializing the state at each step
    NUTS graph are hard to navigate and we agreed this may be a good time to tackle Folding graph representation with Textual aesara#938

@rlouf rlouf force-pushed the assign-aehmc-step branch 3 times, most recently from 51c3d1b to ebfcdcf Compare October 12, 2022 13:31
@rlouf rlouf marked this pull request as ready for review October 12, 2022 13:46
@rlouf rlouf force-pushed the assign-aehmc-step branch from ebfcdcf to 0a586ec Compare October 13, 2022 13:02
@rlouf
Copy link
Member Author

rlouf commented Oct 13, 2022

This is ready for review:

  • The shape issues were solved by specifying the shape of the mass matrix (but this does not solve the issue in Aesara);
  • I moved the discussion about the general interface to Add information about sampling steps for the (human & machine) callers #71; this requires broader changes in the library. It probably requires more discussions and should be its own PR.
  • Adaptation would build on these changes and is thus its own PR as well.
  • I will open an issue to remember to check the graphs produced.

@rlouf rlouf requested a review from brandonwillard October 13, 2022 13:26
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; just some minor comments.

aemcmc/basic.py Outdated Show resolved Hide resolved
tests/test_basic.py Outdated Show resolved Hide resolved
@rlouf rlouf force-pushed the assign-aehmc-step branch from 3a9c92e to 3a783e4 Compare October 18, 2022 07:24
@rlouf rlouf requested a review from zoj613 October 18, 2022 07:25
@rlouf rlouf merged commit 4354b8a into aesara-devs:main Oct 18, 2022
@rlouf rlouf deleted the assign-aehmc-step branch October 18, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assign NUTS sampler to remaining variables
2 participants