Skip to content

Add helper to compute log_likelihood and stop computing it by default #6374

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

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Dec 7, 2022

Closes #6266

Not sure about the function placement...

Major / Breaking Changes

  • sample no longer returns the log_likelihood group by default. Explicitly set idata_kwargs={"log_likelihood": True} or call the helper compute_log_likelihood(idata) after sampling. This is needed when using arviz.compare

@ricardoV94 ricardoV94 added the major Include in major changes release notes section label Dec 7, 2022
@ricardoV94 ricardoV94 added this to the v5.0.0 milestone Dec 7, 2022
@ricardoV94 ricardoV94 changed the title Add helper to compute log_likelihood and stop computing it by default Add helper to compute log_likelihood and stop computing it by default Dec 7, 2022
@ricardoV94 ricardoV94 force-pushed the change_log_likelihood_by_default branch from 041c705 to 65b8bf0 Compare December 7, 2022 12:49
@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #6374 (888fe7b) into main (5692fc0) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6374      +/-   ##
==========================================
+ Coverage   94.71%   94.76%   +0.04%     
==========================================
  Files         132      133       +1     
  Lines       26771    26825      +54     
==========================================
+ Hits        25357    25421      +64     
+ Misses       1414     1404      -10     
Impacted Files Coverage Δ
pymc/tests/sampling/test_forward.py 100.00% <ø> (ø)
pymc/backends/arviz.py 96.37% <100.00%> (+5.65%) ⬆️
pymc/sampling/jax.py 98.19% <100.00%> (ø)
pymc/stats/log_likelihood.py 100.00% <100.00%> (ø)
pymc/tests/backends/test_arviz.py 99.02% <100.00%> (ø)
pymc/tests/sampling/test_jax.py 100.00% <100.00%> (ø)
pymc/tests/stats/test_log_likelihood.py 100.00% <100.00%> (ø)
pymc/backends/base.py 85.38% <0.00%> (-1.93%) ⬇️
pymc/stats/__init__.py

@ricardoV94 ricardoV94 force-pushed the change_log_likelihood_by_default branch from 65b8bf0 to 6b60804 Compare December 7, 2022 13:47
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I would not call the file model comparison. It is not doing any model comparison, it is only an intermediate result that can be used for that, and it is also not specific to model comparison, it is also used in loo_pit which is model checking similar to plot_ppc.

To add to docs, list is somewhere in https://github.com/pymc-devs/pymc/tree/main/docs/source/api. It doesn't really matter if it is "the wrong place" as #5282 is still open and the API docs need significant work.

Most important thing is the comment on dims and coords. If they are indeed preserved, we should add a test for that, if they are not we should fix it and add a test for that.

@ricardoV94 ricardoV94 force-pushed the change_log_likelihood_by_default branch from 6b60804 to c14ad14 Compare December 8, 2022 11:28
@ricardoV94 ricardoV94 requested a review from OriolAbril December 8, 2022 11:28
@ricardoV94 ricardoV94 force-pushed the change_log_likelihood_by_default branch from c14ad14 to f10952a Compare December 8, 2022 12:28
@ricardoV94 ricardoV94 force-pushed the change_log_likelihood_by_default branch from f10952a to 888fe7b Compare December 8, 2022 14:34
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Member

@OriolAbril OriolAbril 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, thanks!

@ricardoV94 ricardoV94 merged commit 5d2c0de into pymc-devs:main Dec 8, 2022
@ricardoV94 ricardoV94 deleted the change_log_likelihood_by_default branch June 6, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Include in major changes release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not compute model log_likelihood by default at the end of sampling
3 participants