Allow xarray.Dataarray input to plots. #1120

smit-s · 2020-03-15T17:21:14Z

Description

This PR is not related to any issue but, as Discussed in #1104 I have made changes to converter.py to allow dataarray input
Consider following code snippet-
import arviz as az
import numpy as np
import xarray as xr
xarr=xr.DataArray(np.random.randn(6,2),name="nm",dims=('chains', 'draw'),coords={'draw':[1,2]})
az.plot_posterior(xarr,var_names="nm")

`
Output without changes-
Traceback (most recent call last):

File "/home/smit/gsoc/repro.py", line 6, in
az.plot_posterior(xarr,var_names="nm")
File "/home/smit/gsoc/local/arv/lib/python3.6/site-packages/arviz-0.7.0-py3.6.egg/arviz/plots/posteriorplot.py", line 184, in plot_posterior
data = convert_to_dataset(data, group=group)
File "/home/smit/gsoc/local/arv/lib/python3.6/site-packages/arviz-0.7.0-py3.6.egg/arviz/data/converters.py", line 177, in convert_to_dataset
inference_data = convert_to_inference_data(obj, group=group, coords=coords, dims=dims)
File "/home/smit/gsoc/local/arv/lib/python3.6/site-packages/arviz-0.7.0-py3.6.egg/arviz/data/converters.py", line 132, in convert_to_inference_data
", ".join(allowable_types), obj.class.name
ValueError: Can only convert xarray dataset, dict, netcdf filename, numpy array, pystan fit, pymc3 trace, emcee fit, pyro mcmc fit, numpyro mcmc fit, cmdstan fit csv filename, cmdstanpy fit to InferenceData, not DataArray

After changes-

Checklist

Follows official PR format
Includes a sample plot to visually illustrate the changes (only for plot-related functions)
Includes new or updated tests to cover the new feature
Code style correct (follows pylint and black guidelines)
Changes are listed in changelog

OriolAbril · 2020-03-15T17:40:29Z

arviz/data/converters.py

@@ -97,6 +99,10 @@ def convert_to_inference_data(obj, *, group="posterior", coords=None, dims=None,
    # Cases that convert to xarray
    if isinstance(obj, xr.Dataset):
        dataset = obj
+    elif isinstance(obj, xr.DataArray):
+        if obj.name is None:
+            obj.name = 'plot'


If the name is None, we should set it to x to follow current convention with numpy arrays.

Yes, true. I will change it.

smit-s · 2020-03-16T16:23:57Z

Do I need to write testcases for this? If yes, please let me know how can I add testcases and what should that test case verify?

OriolAbril · 2020-03-16T18:49:38Z

As this is a new feature it would be great to add some tests for it. Maybe something like TestNumpyToDataArray but starting from a DataArray instead of a numpy array

codecov · 2020-03-21T15:05:11Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1120   +/-   ##
=======================================
  Coverage   92.67%   92.68%           
=======================================
  Files          93       93           
  Lines        9069     9073    +4     
=======================================
+ Hits         8405     8409    +4     
  Misses        664      664

Impacted Files	Coverage Δ
arviz/data/converters.py	`68.57% <100.00%> (+1.90%)`	⬆️

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 f7febc1...4a59e64. Read the comment docs.

update

OriolAbril · 2020-03-22T20:31:10Z

arviz/tests/base_tests/test_data.py

+class TestDataArrayToDataset:
+    def test_1d_dataset(self):
+        size = 100
+        dataset = convert_to_dataset(xr.DataArray(np.random.randn(1, size), dims=('chain', 'draw')))


you could set the name of the input dataarray in this test to make sure the name is not overwritten during conversion

Yes I forgot to test the name. I'll make suitable changes

OriolAbril · 2020-03-22T20:32:34Z

arviz/tests/base_tests/test_data.py

+        assert inference_data.prior.chain.shape == shape[:1]
+        assert inference_data.prior.draw.shape == shape[1:2]
+        assert inference_data.prior[var_name].shape == shape
+        assert repr(inference_data).startswith("Inference data with groups")


this last assert should be removed, it will only fail if the inference data object has not been created, and in this case all asserts before this one would have already failed.

Well observed. I'll change it

OriolAbril

Can you please merge latest changes in master into the PR? There were some issues checking code style with black.

I think after these last changes it will be ready to merge.

OriolAbril · 2020-03-24T15:45:18Z

arviz/tests/base_tests/test_data.py

+        dataset = convert_to_dataset(xr.DataArray(np.random.randn(1, size),
+                                                  name="plot", dims=('chain', 'draw')))
+        assert len(dataset.data_vars) == 1
+        assert dataset.variables.get("plot") is not None


how about assert "plot" in dataset.data_vars?

Ok, I will make changes.
But, where can we see code style issues by black? Because I did not see any suggestion in automation check that runs after each commit.

you can run black --check I believe and it will run black checks. The CI will run black on every commit that is pushed so you'll see a failure on azure pipelines, but its easier just to check locally

Ah nevermind, what I said is how things should ideally run. I saw oriols comment below that CI is not failing on black errors right now

Yes it can easily run locally. I will run all checks before pushing any changes.

canyon289 · 2020-03-26T12:56:42Z

arviz/tests/base_tests/test_data.py

+            dims=('chain', 'draw', 'dim_0', 'dim_1', 'dim_2')), group="prior")
+        assert hasattr(inference_data, "prior")
+        assert len(inference_data.prior.data_vars) == 1
+        var_name = list(inference_data.prior.data_vars)[0]


Nit pick, move this up above the asserts so all the asserts are together. That way when reading the tests we can see all the logic in one place and all the asserts in another. Same for test above

OriolAbril · 2020-03-26T13:04:50Z

You also have to make sure the code is formatted according to black, see contributing guide to check. Code is not formatted with black currently, however, due to #1124, Azure build is not failing even though it should: see https://dev.azure.com/ArviZ/ArviZ/_build/results?buildId=2185&view=logs&j=e6a7683b-6131-58a8-ef68-5f3a9120796c&t=49079c21-ba2f-59ec-b1ba-71c17ead5efa&l=14

smit-s · 2020-03-26T13:41:53Z

Ok, now I got it. I will make sure code is formatted according to black

OriolAbril · 2020-03-26T14:48:53Z

Thanks, I think it is ready to merge now

smit-s added 3 commits March 15, 2020 18:39

Add support for Dataarray

a6c1f97

Comments

93f829d

added condition

20383ce

OriolAbril reviewed Mar 15, 2020

View reviewed changes

review comment

bdf87c6

smit-s added 2 commits March 21, 2020 19:48

test cases for dataarray to dataset

837585e

Resolved pylint corrections

7b311c7

smit-s added 2 commits March 22, 2020 00:00

Merge branch 'master' of github.com:arviz-devs/arviz into dev

8b85c90

update

Add changes to changelog

ebaad2c

smit-s changed the title ~~Allow xarray.Dataarray input to plots.[WIP]~~ Allow xarray.Dataarray input to plots. Mar 21, 2020

OriolAbril reviewed Mar 22, 2020

View reviewed changes

smit-s added 2 commits March 23, 2020 14:39

review comments

1cf8951

pylint error

33648b8

OriolAbril reviewed Mar 24, 2020

View reviewed changes

addressed review comments

b1b3189

canyon289 reviewed Mar 26, 2020

View reviewed changes

smit-s added 2 commits March 26, 2020 18:48

review comments addressed

b9c24af

black formatted

4a59e64

aloctavodia approved these changes Mar 26, 2020

View reviewed changes

aloctavodia merged commit f76da26 into arviz-devs:master Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow xarray.Dataarray input to plots. #1120

Allow xarray.Dataarray input to plots. #1120

smit-s commented Mar 15, 2020 •

edited

Loading

OriolAbril Mar 15, 2020

smit-s Mar 15, 2020

smit-s commented Mar 16, 2020

OriolAbril commented Mar 16, 2020

codecov bot commented Mar 21, 2020 •

edited

Loading

OriolAbril Mar 22, 2020

smit-s Mar 23, 2020

OriolAbril Mar 22, 2020

smit-s Mar 23, 2020

OriolAbril left a comment

OriolAbril Mar 24, 2020

smit-s Mar 24, 2020

canyon289 Mar 30, 2020

canyon289 Mar 30, 2020

smit-s Mar 30, 2020

canyon289 Mar 26, 2020

smit-s Mar 26, 2020

OriolAbril commented Mar 26, 2020

smit-s commented Mar 26, 2020

OriolAbril commented Mar 26, 2020

Allow xarray.Dataarray input to plots. #1120

Allow xarray.Dataarray input to plots. #1120

Conversation

smit-s commented Mar 15, 2020 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smit-s commented Mar 16, 2020

OriolAbril commented Mar 16, 2020

codecov bot commented Mar 21, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril commented Mar 26, 2020

smit-s commented Mar 26, 2020

OriolAbril commented Mar 26, 2020

smit-s commented Mar 15, 2020 •

edited

Loading

codecov bot commented Mar 21, 2020 •

edited

Loading