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

New argument for plotting true values in pairplots #1140

Merged
merged 4 commits into from
Apr 13, 2020
Merged

New argument for plotting true values in pairplots #1140

merged 4 commits into from
Apr 13, 2020

Conversation

agustinaarroyuelo
Copy link
Contributor

@agustinaarroyuelo agustinaarroyuelo commented Apr 6, 2020

Description

This PR adds the argument true_values in plot_pair. It allows for scattering the true values of a variable as suggested in #1131.
true_values_bokeh

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • 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

@ahartikainen
Copy link
Contributor

Hi, do we have some other possible name that true_value? observed_values?

I get the meaning of the true_values, but also somehow it creates a feeling of boolean value.

true_values_copy = {}
label = []
for variable in list(true_values.keys()):
if " " in variable:
Copy link
Contributor

Choose a reason for hiding this comment

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

why there is this step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! I was trying to figure out a way so that the user can specify variable names in an easy manner. For example, simply writing "theta Choate" instead of "theta\nChoate"

CHANGELOG.md Outdated
* Add out-of-sample groups (`predictions` and `predictions_constant_data`) and `constant_data` group to pyro translation #1090
* Add `num_chains` and `pred_dims` arguments to io_pyro #1090
* Integrate jointplot into pairplot, add point-estimate and overlay of plot kinds (#1079)
* Allow xarray.Dataarray input for plots.(#1120)
* Revamped the `hpd` function to make it work with mutidimensional arrays, InferenceData and xarray objects (#1117)
* Skip test for optional/extra dependencies when not installed (#1113)
* Add option to display rank plots instead of trace (#1134)
* Integrate jointplot into pairplot, add point-estimate and overlay of plot kinds #1079
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is repeated 3 times

@@ -140,6 +175,12 @@ def plot_pair(

ax.scatter(pe_x, pe_y, marker="s", s=figsize[0] + 50, **point_estimate_kwargs, zorder=4)

if true_values:
ax.scatter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why scatter instead of plot?

@@ -88,6 +120,9 @@ def plot_pair(
for val, ax_, rotate in ((x, ax_hist_x, False), (y, ax_hist_y, True)):
plot_dist(val, textsize=xt_labelsize, rotated=rotate, ax=ax_, **marginal_kwargs)

ax_hist_x.set_xlim(ax.get_xlim())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was removed recently. It was an error deleting it or adding it again is a mistake?

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 I deleted it. ax_hist_x is created with sharex=ax, therefore axis limits and ticks will be the same, there is no need to manually set both limits after plotting.

@aloctavodia
Copy link
Contributor

I agree the name should me more general. May be ref_value, reference_value, ref_point?

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.

plot_posterior uses ref_val, we could stick with that and try to mimic its structure (i.e. allows several ref values per variable/coord) as much as possible adapting the behaviour?

When I first saw the issue, my first take to approach is was to call 2 times plot_pair using the same axis:

idata = az.load_arviz_data("centered_eight")
ax = az.plot_pair(idata, group="posterior_predictive")
az.plot_pair(idata, group="observed_data", ax=ax, scatter_kwargs=dict(color="red"));

image

which can also be used to plot crossed lines instead of a single marker:

image

which then got me thinking, would it be useful to have some plot_ref_vallike functions? This is probably not the best place to share, but I'll create an issue if there is quorum.

@@ -88,6 +120,9 @@ def plot_pair(
for val, ax_, rotate in ((x, ax_hist_x, False), (y, ax_hist_y, True)):
plot_dist(val, textsize=xt_labelsize, rotated=rotate, ax=ax_, **marginal_kwargs)

ax_hist_x.set_xlim(ax.get_xlim())
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 I deleted it. ax_hist_x is created with sharex=ax, therefore axis limits and ticks will be the same, there is no need to manually set both limits after plotting.

@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #1140 into master will increase coverage by 0.17%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
+ Coverage   92.81%   92.98%   +0.17%     
==========================================
  Files          93       93              
  Lines        9181     9229      +48     
==========================================
+ Hits         8521     8582      +61     
+ Misses        660      647      -13     
Impacted Files Coverage Δ
arviz/plots/pairplot.py 93.90% <ø> (+1.21%) ⬆️
arviz/plots/backends/bokeh/pairplot.py 84.31% <91.66%> (+9.11%) ⬆️
arviz/plots/backends/matplotlib/pairplot.py 90.18% <91.66%> (+4.57%) ⬆️

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 3352a8a...ee1b316. Read the comment docs.

ahartikainen and others added 4 commits April 9, 2020 12:51
…es (#1079)

* add jointplot features into pairplot

* add scatter_kde kind for pairplot

* add point_estimate arguments

* bokeh backend

* fix None argument for color in kdeplot bokeh backend

* run black, pylint and pytest

* remove scatter_kde kind among several other changes

* minor changes

* run pytest

* add plot width and height to backend_kwargs

fix pylint issues

fix hover feature

fix hover feature

minor fixes

* update docstring

run pylint

* update changelog
change argument name to reference_values
@agustinaarroyuelo agustinaarroyuelo changed the title [WIP] New argument for plotting true values in pairplots New argument for plotting true values in pairplots Apr 9, 2020
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

@aloctavodia aloctavodia merged commit 4486db8 into arviz-devs:master Apr 13, 2020
@agustinaarroyuelo agustinaarroyuelo deleted the true_values branch April 14, 2020 00:05
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.

4 participants