Addressing issue number #470 (using plot_kde with inference data object) #600

Ban-zee · 2019-02-26T22:59:30Z

Made the following changes:

Using inference data or xr.dataset with plot_kde now raises an error prompting the user to use plot_posterior instead.
Rug plots displayed by default for plot_posterior plots.
Not included the tests yet.

Ban-zee · 2019-02-28T15:21:14Z

pylint failing because of the following :
E:301,17: Instance of 'InferenceData' has no 'posterior' member (no-member)
for the following code:
with pytest.raises(ValueError, match="Xarray"): plot_kde(eight.posterior)
Any idea on how to resolve this issue?

canyon289 · 2019-02-28T15:24:20Z

The first one you likely need to disable

The second one might be a real error. I would check the exception to ensure its the correct one thats being raised. The pytest documentation can help you understand how the exception is being handled
https://docs.pytest.org/en/latest/assert.html#assertions-about-expected-exceptions

canyon289 · 2019-02-28T15:27:29Z

arviz/tests/test_plots.py

@@ -6,7 +6,7 @@
 import numpy as np
 import pytest
 import pymc3 as pm
-
+import arviz as az


This should be a relative import from arviz, not an absolute import

pylint keeps throwing the following exception: Inference Data has no 'posterior' member. Any explanation as to why this happens?

Its because az.InferenceData has no posterior member :) at least during static code introspection.

If you look at the code for InferenceData posterior isn't explicitly called out, it's generated during initialization of the class. But pylint only statically analyzes code so when it "looks" it can't find anything and throws the exception.

In our case that error should be surpressed. The reason you in particular are getting so many issues is because pylint released a new version in the last couple of days, and now this PR is getting hit with all the linting errors.

My suggestion to you is to ignore the linting errors in CI for now. Just make sure it passes in your local environment. Someone will likely make a PR in the next couple of days that handles all the new linting rules, at which point it'll get merged, and this PR can merge or rebase in the new changes. Hope that helps

https://github.com/arviz-devs/arviz/blob/master/arviz/data/inference_data.py#L24

The tests do pass on my local environment. Thank you for the explanation. In the meantime, I'll start working on a different issue.

aloctavodia · 2019-03-01T20:48:01Z

arviz/plots/kdeplot.py

@@ -146,6 +147,10 @@ def plot_kde(

    figsize, *_, xt_labelsize, linewidth, markersize = _scale_fig_size(figsize, textsize, 1, 1)

+    if isinstance(values, xr.Dataset):
+        raise ValueError("Xarray dataset object detected. Use plot_posterior instead of plot_kde")


I would suggest the user to use plot_posterior, plot_density, plot_joint or plot_pair.

aloctavodia · 2019-03-01T20:49:23Z

arviz/plots/posteriorplot.py

@@ -358,6 +358,7 @@ def format_axes():
            fill_kwargs={"alpha": kwargs.pop("fill_alpha", 0)},
            plot_kwargs={"linewidth": linewidth},
            ax=ax,
+            rug=True,


The default value should be False, just to keep this plot close to the originals by Kruschke

ahartikainen · 2019-03-04T11:36:19Z

This PR needs to be rebased against the current Master. It contains now many commits from previous PRs.

This probably should work with forks:

So in your ArviZ folder (forked repo) typed to terminal

First select master branch

git checkout master
git remote add upstream git://github.com/arviz-devs/arviz.git
git fetch upstream
git pull upstream master

Then select your branch

git checkout i-470
git rebase master

You can check your log that everything looks good.

git log --oneline

Push with force to update repo.

git push -f

* Using inference data or xr.dataset with plot_kde now raises an error prompting user to use plot_posterior instead. * Rug plots displayed by default for plot_posterior plots. * Not included the tests yet.

Ban-zee · 2019-03-04T12:12:03Z

Hmm, the commits were copied, should I revert back to a previous commit and push again?

ahartikainen · 2019-03-04T13:06:40Z

That looks correct? Those are commits belonging to this PR right?

Ban-zee · 2019-03-04T13:41:55Z

Yes, these commits belong to the same PR. It was an error on my part as I just read the commit messages and assumed that the commits were somehow copied :(.

aloctavodia · 2019-03-05T14:18:02Z

LGTM!

canyon289 · 2019-03-05T14:49:12Z

arviz/plots/kdeplot.py

+        raise ValueError(
+            "Xarray dataset object detected.Use plot_posterior,plot_denstiy,plot_joint or plot_pair"
+            "instead of "
+            "plot_kde"


Seems weird there's an extra line break here. Doesn't hurt anything but just curious why its there?

The initial line was too long and pylint was failing. Added an extra line for the aesthetics :).

Just to make sure I understand "instead of plot_kde" was added for aesthetics?

canyon289 · 2019-03-05T14:49:35Z

arviz/plots/kdeplot.py

@@ -146,6 +147,14 @@ def plot_kde(

    figsize, *_, xt_labelsize, linewidth, markersize = _scale_fig_size(figsize, textsize, 1, 1)

+    if isinstance(values, xr.Dataset):
+        raise ValueError(
+            "Xarray dataset object detected.Use plot_posterior,plot_denstiy,plot_joint or plot_pair"


Density is spelled incorrectly, spaces between the different plots would be helpful when reading

I'll make the changes.

No worries, this PR is basically at the finish line. Very minor things.

Thanks so much for the contribution

Ban-zee · 2019-03-05T16:25:01Z

Made the changes.

canyon289 · 2019-03-06T16:18:55Z

arviz/tests/test_plots.py

@@ -7,7 +7,7 @@
 import pytest
 import pymc3 as pm

-
+from ..data import load_arviz_data


Being nitpicky here, can this import be combined with the import from ..data below?

canyon289 · 2019-03-06T16:19:27Z

arviz/tests/test_plots.py

@@ -313,6 +313,14 @@ def test_plot_kde_quantiles(continuous_model, kwargs):
    assert axes


+def test_plot_kde_inference_data():
+    eight = load_arviz_data("centered_eight")


Docstring for this test would be great even if its oneline. I know we don't have docstrings for all tests but I'm trying to reverse that trend.

canyon289 · 2019-03-06T16:20:29Z

@Ban-zee Apologies for not getting all the comments in on the last round of review. I think you're just about there after these last changes!

canyon289 · 2019-03-07T19:43:07Z

Thanks for help!

Ban-zee changed the title ~~Made the following additions:~~ Addressing issue number #470 (using plot_kde with inference data object) Feb 26, 2019

canyon289 reviewed Feb 28, 2019

View reviewed changes

aloctavodia reviewed Mar 1, 2019

View reviewed changes

Ban-zee added 8 commits March 4, 2019 17:20

Made the following additions:

e672cf6

* Using inference data or xr.dataset with plot_kde now raises an error prompting user to use plot_posterior instead. * Rug plots displayed by default for plot_posterior plots. * Not included the tests yet.

Added the test

e941d24

Made the suggested changes.

904843e

fixed load_arviz_data import

f028d44

Made the following additions:

dd60007

* Using inference data or xr.dataset with plot_kde now raises an error prompting user to use plot_posterior instead. * Rug plots displayed by default for plot_posterior plots. * Not included the tests yet.

Added the test

6f9e940

Made the suggested changes.

3ba11db

fixed load_arviz_data import

ed78370

Ban-zee force-pushed the i-470 branch from 5cbc5b3 to ed78370 Compare March 4, 2019 12:09

canyon289 reviewed Mar 5, 2019

View reviewed changes

Made the suggested changes.

57d57b5

canyon289 reviewed Mar 6, 2019

View reviewed changes

Added a docstring to my test.

0f475d1

canyon289 merged commit 99dd219 into arviz-devs:master Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing issue number #470 (using plot_kde with inference data object) #600

Addressing issue number #470 (using plot_kde with inference data object) #600

Ban-zee commented Feb 26, 2019 •

edited

Loading

Ban-zee commented Feb 28, 2019

canyon289 commented Feb 28, 2019

canyon289 Feb 28, 2019

Ban-zee Feb 28, 2019

canyon289 Feb 28, 2019

Ban-zee Mar 1, 2019

aloctavodia Mar 1, 2019 •

edited

Loading

aloctavodia Mar 1, 2019

ahartikainen commented Mar 4, 2019

Ban-zee commented Mar 4, 2019

ahartikainen commented Mar 4, 2019

Ban-zee commented Mar 4, 2019 •

edited

Loading

aloctavodia commented Mar 5, 2019

canyon289 Mar 5, 2019

Ban-zee Mar 5, 2019

canyon289 Mar 5, 2019

Ban-zee Mar 5, 2019

canyon289 Mar 5, 2019

Ban-zee Mar 5, 2019 •

edited

Loading

canyon289 Mar 5, 2019

Ban-zee commented Mar 5, 2019

canyon289 Mar 6, 2019

canyon289 Mar 6, 2019

canyon289 commented Mar 6, 2019

canyon289 commented Mar 7, 2019

Addressing issue number #470 (using plot_kde with inference data object) #600

Addressing issue number #470 (using plot_kde with inference data object) #600

Conversation

Ban-zee commented Feb 26, 2019 • edited Loading

Ban-zee commented Feb 28, 2019

canyon289 commented Feb 28, 2019

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

aloctavodia Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahartikainen commented Mar 4, 2019

Ban-zee commented Mar 4, 2019

ahartikainen commented Mar 4, 2019

Ban-zee commented Mar 4, 2019 • edited Loading

aloctavodia commented Mar 5, 2019

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

Ban-zee Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ban-zee commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canyon289 commented Mar 6, 2019

canyon289 commented Mar 7, 2019

Ban-zee commented Feb 26, 2019 •

edited

Loading

aloctavodia Mar 1, 2019 •

edited

Loading

Ban-zee commented Mar 4, 2019 •

edited

Loading

Ban-zee Mar 5, 2019 •

edited

Loading