Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[proposal] support for IC with multiple variables #1173

Merged
merged 6 commits into from
May 16, 2020
Merged

Conversation

OriolAbril
Copy link
Member

@OriolAbril OriolAbril commented May 4, 2020

Description

Given the number of possibilities and the even larger number of things that can go wrong, I propose to add only a var_name argument (or name/log_likelihood to avoid confusion with var_names that can be a list) and force users to manually add the combined log likelihood data on which they want to calculate loo/waic to log_likelihood group.

We could add some var_names arg instead and a combine arg to sum or concat or maybe even try to cover groupby too, but this will always be more limited that creating the new variable and it will also be more obscure (i.e. two variables with the default coord names var1_dim_0 and var2_dim_0 will be broadcasted to a 2d array when added because their coord names are different, if the user has to manually create the dataarray of pointwise log likelihood, this can be seen before passing it to loo/waic). I find that both xarray and loo/waic are quite obscure and commonly misunderstood subjects to support this choice. The overhead to users I anticipate is checking examples to copy the combine code most similar to their situation which I actually see like an advantage as it will force them to think about which of the examples better fits their particular case.

closes (maybe) #998 and #987. By maybe I mean that everything will be possible but we will still be missing detailed examples and guidance on how to do it.

Checklist

  • Follows official PR format
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #1173 into master will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
+ Coverage   93.05%   93.23%   +0.18%     
==========================================
  Files          94       94              
  Lines        9376     9376              
==========================================
+ Hits         8725     8742      +17     
+ Misses        651      634      -17     
Impacted Files Coverage Δ
arviz/rcparams.py 93.10% <ø> (ø)
arviz/stats/stats_utils.py 95.66% <ø> (+3.94%) ⬆️
arviz/stats/stats.py 96.20% <100.00%> (+0.35%) ⬆️
arviz/data/io_dict.py 92.92% <0.00%> (+3.03%) ⬆️

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 6468f64...4400ad2. Read the comment docs.

@canyon289
Copy link
Member

At a high level I think making things more explicit is good. That being said this is at a depth of diagnostics I dont quite understand. I'll check out this branch locally and give it a try

@OriolAbril
Copy link
Member Author

I think this is ready to merge, it would be great to include in the upcoming release

Copy link
Contributor

@aloctavodia aloctavodia left a comment

Choose a reason for hiding this comment

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

LGTM. Totally agree on giving user the responsibility. This can be merged once the changelog is updated. We should definitely add a few examples, but that can be done in another PR.

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.

LGTM, thanks Oriol! I only saw one tiny typo. I'll let it up to you if you want to fix it here or in another PR of yours.

@canyon289
Copy link
Member

Looks great. Merging

@canyon289 canyon289 merged commit 4192ad0 into master May 16, 2020
@OriolAbril OriolAbril deleted the ic_var_name branch May 16, 2020 22:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants