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

Implement log-probability for CumSum Op #72

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

ricardoV94
Copy link
Contributor

@ricardoV94 ricardoV94 commented Oct 14, 2021

import aesara.tensor as at
from aeppl import joint_logprob

rv = at.random.normal(0, 1, size=5).cumsum()
vv = rv.clone()

logp = joint_logprob({rv: vv}, sum=False)
logp.eval({vv: np.arange(1, 6)})
# array([-1.41893853, -1.41893853, -1.41893853, -1.41893853, -1.41893853])

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #72 (ad37be1) into main (9298dac) will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   94.78%   94.92%   +0.13%     
==========================================
  Files           8        9       +1     
  Lines        1227     1260      +33     
  Branches      163      164       +1     
==========================================
+ Hits         1163     1196      +33     
  Misses         31       31              
  Partials       33       33              
Impacted Files Coverage Δ
aeppl/cumsum.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 9298dac...ad37be1. Read the comment docs.

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, but I don't think the CumSum code should be in scan.py—that module is pretty specific to Scan.

In general, given the extent to which we're likely to expand Aesara Op coverage (i.e. a large extent), we should start creating new modules. For example, we could start mirroring the modules in which the Aesara Ops are defined—at least when we plan on having log-probability implementations for more than one Op from the same Aesara modules.

aeppl/scan.py Outdated Show resolved Hide resolved
@brandonwillard brandonwillard added enhancement New feature or request graph rewriting Involves the implementation of rewrites to Aesara graphs op-probability Involves the implementation of log-probabilities for Aesara `Op`s labels Oct 14, 2021
@ricardoV94
Copy link
Contributor Author

ricardoV94 commented Oct 14, 2021

Looks good, but I don't think the CumSum code should be in scan.py—that module is pretty specific to Scan.

Yeah, I was not very happy with it tucked in there either. Aesara puts this is extra_ops though, should we do the same or something more direct?

Edit: Otherwise, we can organize by conceptual types of RVs... like the mixtures. In that case there was a slight link between this and the scan rewrites

@brandonwillard
Copy link
Member

Yeah, I was not very happy with it tucked in there either. Aesara puts this is extra_ops though, should we do the same or something more direct?

An extra_ops module is fine for now.

Edit: Otherwise, we can organize by conceptual types of RVs... like the mixtures.

Exactly

aeppl/scan.py Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 force-pushed the cumsum_rvs branch 2 times, most recently from d5bf8c3 to eb01b9c Compare October 28, 2021 15:11
@ricardoV94 ricardoV94 marked this pull request as ready for review October 28, 2021 15:12
@ricardoV94 ricardoV94 force-pushed the cumsum_rvs branch 2 times, most recently from c9583a6 to 3721964 Compare October 29, 2021 06:45
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 great!

@brandonwillard brandonwillard merged commit 18275e2 into aesara-devs:main Nov 2, 2021
@brandonwillard brandonwillard changed the title Cumsum rvs Implement log-probability for CumSum Op Nov 10, 2021
@ricardoV94 ricardoV94 deleted the cumsum_rvs branch December 16, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request graph rewriting Involves the implementation of rewrites to Aesara graphs op-probability Involves the implementation of log-probabilities for Aesara `Op`s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants