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

skip "event" dims in log likelihood group whenever necessary #1429

Merged
merged 7 commits into from
Oct 26, 2020

Conversation

OriolAbril
Copy link
Member

@OriolAbril OriolAbril commented Oct 21, 2020

Description

ArviZ schema recommends to use the same variable names in log likelihood group as the one in observed_data and posterior_predictive groups. This helps identifying to what terms does each pointwise log lik value correspond, and eases calculations like LOO-PIT.

However, log likelihood and posterior predictive don't always have the same dimensions and shape. In multidimensional observations the event dimensions (shape of a single observation) are reduced. The shape of posterior predictive is (chain, draw, *batch_shape, *event_shape) whereas for log likelihood it is (chain, draw, *batch_shape), in scalar observations they are both the same, but for multidimensional observations such as multivariate normals or multinomials the two shapes are different and the converters error out.

This will automatically handle this event_shape difference provided that they follow this convention, that is, event dims are the last ones.

cc @AlexAndorra

Checklist

  • Follows official PR format
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

Thanks @OriolAbril, LGTM! I just spotted a doctring that I think could be more explicit, and still have to check that my PyMCon model works with this change now -- will keep you posted

arviz/data/base.py Outdated Show resolved Hide resolved
@OriolAbril OriolAbril changed the title [WIP] skip "event" dims in log likelihood group whenever necessary skip "event" dims in log likelihood group whenever necessary Oct 23, 2020
@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #1429 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1429   +/-   ##
=======================================
  Coverage   91.58%   91.59%           
=======================================
  Files         105      105           
  Lines       11125    11136   +11     
=======================================
+ Hits        10189    10200   +11     
  Misses        936      936           
Impacted Files Coverage Δ
arviz/data/io_cmdstan.py 91.97% <ø> (ø)
arviz/data/io_cmdstanpy.py 98.19% <ø> (ø)
arviz/data/io_dict.py 92.91% <ø> (ø)
arviz/data/io_pymc3.py 92.44% <ø> (ø)
arviz/data/io_pystan.py 96.00% <ø> (ø)
arviz/data/base.py 97.70% <100.00%> (+0.20%) ⬆️
arviz/data/io_numpyro.py 95.27% <100.00%> (ø)
arviz/data/io_pyro.py 97.35% <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 5e50848...75e2cd3. Read the comment docs.

@OriolAbril
Copy link
Member Author

This should be ready to merge

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @OriolAbril !
I haven't had time to test it on my PyMCon model yet (as I'm still using a fork of the repo, checking out PRs is always a bit of a pain; I should just clone the actual repo now FWIW) but as you did it on the Labs code, I suggest we merge and then I'll test when it's on the master branch?
Also, why is Azure not showing up here?

@OriolAbril
Copy link
Member Author

OriolAbril commented Oct 26, 2020

I haven't had time to test it on my PyMCon model yet (as I'm still using a fork of the repo, checking out PRs is always a bit of a pain; I should just clone the actual repo now FWIW) but as you did it on the Labs code, I suggest we merge and then I'll test when it's on the master branch?

I added a test on a pymc3 multinomial to be extra sure, the rest of the tests are backend agnostic at a lower level, so don't worry about it. For future reference, to quickly try the code of a PR (provided it's a PR created from a branch of the main repo) you can do pip install git+https://github.com/arviz-devs/arviz@<branch-name>, you won't be able to edit the source code and push back, but for quick testing is quite convenient.

Also, why is Azure not showing up here?

No idea, we had some issues with Azure last week, hopefully they will have solved themselves by now (we'll need a new PR to see). Azure has run and passed though, it's only an issue github reporting

@AlexAndorra
Copy link
Contributor

Sounds good! I'll go ahead and merge then

@AlexAndorra AlexAndorra merged commit ead5c98 into master Oct 26, 2020
@AlexAndorra AlexAndorra deleted the multidim_obs branch October 26, 2020 12:09
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