Skip to content

Add user-friendly logp and logcdf methods #4833

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

Merged
merged 3 commits into from
Jul 6, 2021

Conversation

ricardoV94
Copy link
Member

This PR adds a logp helper similar to the logcdf that already existed to facilitate the extraction of logp and logcdf expressions, similar to how the .logp and .logcdf methods worked in v3:

import pymc3 as pm

# v3:
logp_norm = pm.Normal.dist(0, 1).logp(5.0)
locdf_norm = pm.Normal.dist(0, 1).logcdf(5.0)

# v4:
logp_norm = pm.logp(pm.Normal.dist(0, 1), 5.0)
logcdf_norm = pm.logcdf(pm.Normal.dist(0, 1), 5.0)

The file distributions/logp.py was renamed to distributions/logprob.py to avoid the the name conflict.

@ricardoV94 ricardoV94 changed the title Add friendly logp and logcdf methods Add user-friendly logp and logcdf methods Jul 2, 2021
@ricardoV94 ricardoV94 requested a review from brandonwillard July 2, 2021 10:19
@ricardoV94 ricardoV94 force-pushed the add_logp_helper branch 2 times, most recently from 0792aaa to 7f60337 Compare July 2, 2021 15:37
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

I like it!

That reduces one breaking v3v4 change to something less painful:

my_rv = pm.Normal.dist(0, 2)

# on v3:
my_rv.logp(1.5).eval()

# on v4.
pm.logp(my_rv, 1.5).eval()

@michaelosthege michaelosthege requested a review from twiecki July 2, 2021 16:46
Copy link
Contributor

@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.

Generally speaking, don't do too much with respect to the likelihood calculations in PyMC, because we're ultimately going to set PyMC up to use aeppl. Since this looks more like a PyMC-specific interface change, that might not be an issue, though.

@ricardoV94
Copy link
Member Author

Generally speaking, don't do too much with respect to the likelihood calculations in PyMC, because we're ultimately going to set PyMC up to use aeppl. Since this looks more like a PyMC-specific interface change, that might not be an issue, though.

Yeah, this one is actually mimicking a bit what we have in aeppl just to extract a logp/logcdf of a vanilla RV

@ricardoV94 ricardoV94 force-pushed the add_logp_helper branch 3 times, most recently from fbb0033 to d6866fc Compare July 4, 2021 14:03
@codecov
Copy link

codecov bot commented Jul 4, 2021

Codecov Report

Merging #4833 (528edc7) into main (223b0b8) will increase coverage by 0.01%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4833      +/-   ##
==========================================
+ Coverage   72.31%   72.32%   +0.01%     
==========================================
  Files          85       85              
  Lines       13877    13884       +7     
==========================================
+ Hits        10035    10042       +7     
  Misses       3842     3842              
Impacted Files Coverage Δ
pymc3/distributions/logprob.py 95.89% <88.88%> (ø)
pymc3/distributions/__init__.py 100.00% <100.00%> (ø)
pymc3/distributions/discrete.py 99.00% <100.00%> (ø)
pymc3/step_methods/hmc/base_hmc.py 91.05% <0.00%> (+0.81%) ⬆️

@ricardoV94 ricardoV94 force-pushed the add_logp_helper branch 2 times, most recently from 40826e2 to e35217b Compare July 5, 2021 11:42
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

bad luck

@michaelosthege michaelosthege merged commit a061106 into pymc-devs:main Jul 6, 2021
@ricardoV94 ricardoV94 deleted the add_logp_helper branch August 3, 2021 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants