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

[WIP] Add new groups to from_numpyro and from_dict #1125

Merged
merged 8 commits into from
Apr 16, 2020

Conversation

nitishp25
Copy link
Contributor

@nitishp25 nitishp25 commented Mar 26, 2020

Description

Fixes #1030

Add the following groups:

  • predictions
  • constant_data (to from_numpyro only)
  • predictions_constant_data

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 Mar 26, 2020

Codecov Report

Merging #1125 into master will increase coverage by 0.12%.
The diff coverage is 89.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1125      +/-   ##
==========================================
+ Coverage   92.99%   93.12%   +0.12%     
==========================================
  Files          94       94              
  Lines        9242     9283      +41     
==========================================
+ Hits         8595     8645      +50     
+ Misses        647      638       -9     
Impacted Files Coverage Δ
arviz/data/io_pymc3.py 92.64% <ø> (ø)
arviz/data/io_pyro.py 95.86% <ø> (ø)
arviz/data/io_dict.py 89.89% <71.42%> (+7.24%) ⬆️
arviz/data/io_numpyro.py 95.23% <97.82%> (+3.64%) ⬆️

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 d1e1a2a...ac52779. Read the comment docs.

@nitishp25
Copy link
Contributor Author

@OriolAbril what do you think? Also, can I update from_dict in this PR or separately?

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.

Overall looks good, I'll try to review more in depth in the following days.

There would be no problem updating from_dict here too as it would also make sense to do another PR for it, whatever you prefer.


def test_inference_data_only_posterior(self, data):
idata = from_numpyro(data.obj)
test_dict = {"posterior": ["mu", "tau", "eta"], "sample_stats": ["diverging"]}
Copy link
Member

Choose a reason for hiding this comment

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

I think log_likelihood will also be present with only posterior

@nitishp25 nitishp25 changed the title [WIP] Add new groups to from_numpyro [WIP] Add new groups to from_numpyro and from_dict Mar 28, 2020
@@ -156,6 +169,26 @@ def constant_data_to_xarray(self):
constant_data[key] = xr.DataArray(vals, dims=val_dims, coords=coords)
return xr.Dataset(data_vars=constant_data, attrs=make_attrs(library=None))

@requires("predictions_constant_data")
def predictions_constant_data_to_xarray(self):
Copy link
Member

Choose a reason for hiding this comment

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

If it is not too much to ask, no pressure, could you do the same trick here as with constant data and predictions constant data in from_pyro and from_numpyro? If I am not mistaken, the code here is actually shared not between the 2 constant data group but also with observed data group, it would significantly reduce code duplication

Copy link
Contributor Author

@nitishp25 nitishp25 Mar 28, 2020

Choose a reason for hiding this comment

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

Of course! It makes much more sense. Thanks! Should I also do this for other groups like posterior_predictive, predictions, prior_predictive etc?

Copy link
Member

Choose a reason for hiding this comment

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

It could be done, I am not sure it is worth it though, they are already 3 lines long, the bulk of the code is already the external function dict_to_dataset, the only difference between them is the error.

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.

LGTM

Comment on lines 106 to 107
"""When constructing InferenceData must have at least
one of posterior, prior, posterior_predictive or predictions."""
Copy link
Member

Choose a reason for hiding this comment

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

I think using """ will keep all the spaces at the beginning of the line, formatting the output in a strange way, this should not keep the spaces:

                raise ValueError(
                    "When constructing InferenceData must have at least"
                    "one of posterior, prior, posterior_predictive or predictions."

@OriolAbril
Copy link
Member

I think it is ready to merge

@OriolAbril OriolAbril merged commit 1e3d25d into arviz-devs:master Apr 16, 2020
nitishp25 added a commit to nitishp25/arviz that referenced this pull request Apr 27, 2020
* added groups

* update tests and changelog

* lint changes

* update from_dict and tests

* update changelog

* modify io_dict

* minor fixes

* update changelog again

Add warning for log scale default in compare/loo/waic functions (arviz-devs#1150)

* Added scale warning to ELPDData

* Added scale warning to compare function

* Added changes to Changelog

* Moved warning to end of string in loo function

* Removed last test for loo_print

* Ran Black

* Integrated Oriol's comments

add local to docstring, and use lowercase for ess (arviz-devs#1152)

hardcode show=False in plot_posterior subplots (arviz-devs#1151)

* hardcode show=False in plot_posterior subplots

* update changelog

Fix documentation and deprecation warning in pair plot (arviz-devs#1156)

The "kind" argument is not described correctly.

* Fix pairplot warning.

Previously, because of incorrect argument checking, pairplot would
mistakenly warn the caller not to use the "contour" argument when that
argument was NOT supplied.  Changed default value to None and did the
defaulting by hand to fix this issue.

Also added some type declarations.

* Clarified docstring for contour argument.

Co-authored-by: Robert P. Goldman <[email protected]>

add viridis as default cmap (arviz-devs#1160)

add examples to customize 2D KDE (arviz-devs#1158)

* add examples to customize 2D KDE

* blackify gallery example

* update changelog

* briefly explain contour_kwargs and contourf_kwargs
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.

New predictions group!
2 participants