-
-
Notifications
You must be signed in to change notification settings - Fork 423
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
Addressing issue number #470 (using plot_kde with inference data object) #600
Changes from 8 commits
e672cf6
e941d24
904843e
f028d44
dd60007
6f9e940
3ba11db
ed78370
57d57b5
0f475d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,8 @@ | |
from scipy.signal import gaussian, convolve, convolve2d # pylint: disable=no-name-in-module | ||
from scipy.sparse import coo_matrix | ||
from scipy.stats import entropy | ||
|
||
import xarray as xr | ||
from ..data.inference_data import InferenceData | ||
from ..utils import conditional_jit | ||
from .plot_utils import _scale_fig_size | ||
|
||
|
@@ -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" | ||
"instead of " | ||
"plot_kde" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems weird there's an extra line break here. Doesn't hurt anything but just curious why its there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The initial line was too long and pylint was failing. Added an extra line for the aesthetics :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to make sure I understand "instead of plot_kde" was added for aesthetics? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
) | ||
if isinstance(values, InferenceData): | ||
raise ValueError(" Inference Data object detected. Use plot_posterior instead of plot_kde") | ||
if values2 is None: | ||
if plot_kwargs is None: | ||
plot_kwargs = {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
import pytest | ||
import pymc3 as pm | ||
|
||
|
||
from ..data import load_arviz_data | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Being nitpicky here, can this import be combined with the import from |
||
from ..data import from_dict, from_pymc3 | ||
from ..stats import compare, psislw | ||
from .helpers import eight_schools_params, load_cached_models # pylint: disable=unused-import | ||
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
with pytest.raises(ValueError, match="Inference Data"): | ||
plot_kde(eight) | ||
with pytest.raises(ValueError, match="Xarray"): | ||
plot_kde(eight.posterior) | ||
|
||
|
||
def test_plot_khat(): | ||
linewidth = np.random.randn(20000, 10) | ||
_, khats = psislw(linewidth) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Density is spelled incorrectly, spaces between the different plots would be helpful when reading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, this PR is basically at the finish line. Very minor things.
Thanks so much for the contribution