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

Fix check for missing rv in factorized_joint_logprob #151

Merged
merged 2 commits into from
Jul 5, 2022

Conversation

larryshamalama
Copy link
Contributor

@larryshamalama larryshamalama commented Jun 30, 2022

Currently, random variables that are not provided with their value variable equivalent can result in a KeyError in factorized_joint_logprob when given warn_missing_rvs=False. This error rose when working with a graph corresponding to a Gaussian random walk.

import aesara.tensor as at
from aeppl import factorized_joint_logprob

mu = at.random.normal(loc=2, scale=0.1)
sigma = at.random.gamma(shape=1, rate=1)
innovation_dist = at.random.normal(mu[..., None], sigma[..., None], size=(10,))
rv_out = at.cumsum(innovation_dist, axis=-1) # gaussian random walk

The following code chunk yields a KeyError:

factorized_joint_logprob(
    {
        rv_out: rv_out.copy(),
    }, 
    extra_rewrites=TransformValuesOpt({}),
    use_jacobian=True,
    warn_missing_rvs=False
)

but setting warn_missing_rvs=True only issues warnings.

In this PR, so far, I fix the case where users decide to not issue warnings.

@ricardoV94
Copy link
Contributor

Can you add a test? Your example should be fine just assert that the warning raised in one case but not the other, but the output is the same.

@larryshamalama
Copy link
Contributor Author

larryshamalama commented Jun 30, 2022

Done! There was already a test for this, I just added a verification for warn_missing_rvs=False. Here, is it better to have both commits as one?

ricardoV94
ricardoV94 previously approved these changes Jul 4, 2022
@larryshamalama
Copy link
Contributor Author

Failing test as described in #148:

def test_initial_values():

@ricardoV94
Copy link
Contributor

ricardoV94 commented Jul 5, 2022

You can just cherry pick (or copy) this commit: e0f9623

@larryshamalama
Copy link
Contributor Author

You can just cherry pick (or copy) this commit: e0f9623

Done!

@ricardoV94
Copy link
Contributor

You should have the two commits separate.

One for the xfail and the other for the bugfix and new test condition.

@larryshamalama larryshamalama force-pushed the fix-joint-logprob branch 2 times, most recently from b82d518 to 2ff42f3 Compare July 5, 2022 15:49
@larryshamalama
Copy link
Contributor Author

You should have the two commits separate.

One for the xfail and the other for the bugfix and new test condition.

Done! Thanks for the clarification

@ricardoV94
Copy link
Contributor

ricardoV94 commented Jul 5, 2022

Small nitpick: the test and respective bugfix make sense to go in the same commit

@larryshamalama
Copy link
Contributor Author

larryshamalama commented Jul 5, 2022

Small nitpick: the test and respective bugfix make sense to go in the same commit

After several attempts and many force pushes, I think that I got it right (?) 😅

@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #151 (832e329) into main (cc78f30) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #151   +/-   ##
=======================================
  Coverage   94.88%   94.88%           
=======================================
  Files          12       12           
  Lines        1779     1780    +1     
  Branches      262      263    +1     
=======================================
+ Hits         1688     1689    +1     
  Misses         51       51           
  Partials       40       40           
Impacted Files Coverage Δ
aeppl/joint_logprob.py 96.92% <100.00%> (+0.04%) ⬆️

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 cc78f30...832e329. Read the comment docs.

@ricardoV94 ricardoV94 merged commit e8f89ea into aesara-devs:main Jul 5, 2022
@larryshamalama larryshamalama deleted the fix-joint-logprob branch July 5, 2022 21:06
maresb added a commit to maresb/pymc that referenced this pull request Jul 7, 2022
This adapts to the change in behavior introduced in
aesara-devs/aeppl#151.
ricardoV94 pushed a commit to pymc-devs/pymc that referenced this pull request Jul 7, 2022
* Bump Aesara to v2.7.5

* Bump aeppl to v0.0.32

* Revert warn_missing_rvs to default value of True

See #5955 (comment)

This adapts to the change in behavior introduced in
aesara-devs/aeppl#151.
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.

2 participants