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

Traceplot fixes #679

Merged
merged 15 commits into from May 30, 2019

Conversation

@OriolAbril
Copy link
Contributor

commented May 24, 2019

For now still minor fixes. Still not gone into facet/compact styles. Should fix #654.

EDIT
Already gone into facet/compact styles. Reviews and advise welcome. The idea is to end up fixing both #94 and #609.

arviz/plots/traceplot.py Outdated Show resolved Hide resolved

OriolAbril added some commits May 26, 2019

Update get_coords docstring
Specify that it works for both xr.DataSet and xr.DataArray
@OriolAbril

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Now plot_trace should be compatible with all kind of coords, even when plotting divergences, and in addition, the selected draws will be reflected in the xaxis. I modified the combined behaviour to combine the samples only on the distplot, but this can be modified. Below there are 2 examples of the centered schools data trace followed by the previous behaviour:

az.plot_trace(idata, var_names=["tau", "mu"]);

Current behaviour:

new behaviour

which is the same as before this changes:

old behaviour

az.plot_trace(idata, var_names=["tau", "mu"], combined=True, coords={"draw":slice(100,None,3), "chain":slice(1,None)}, legend=True);

Current behaviour:

new_plot_trace

in this case, the previus version does not allow all this options, thus, after modifying some of them to avoid an error:

az.plot_trace(idata, var_names=["tau", "mu"], combined=True, coords={"draw":slice(100,None), "chain":slice(1,None)}, divergences=False);

old_plot_trace

Note that even though only the last 400 draws are used, the axis go from 0 to 500, and there are always values, because as the array is flattened, they reach up to 1200. What is seen above is the first chain and part of the second:

old_plot_trace2-axis

@@ -16,7 +16,9 @@ def plot_trace(
figsize=None,
textsize=None,
lines=None,
compact=False,

This comment has been minimized.

Copy link
@aloctavodia

aloctavodia May 26, 2019

Contributor

I agree this should be False by default, we should also have a max number of plots by default.

@ahartikainen

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

Do we need top, left, right, (bottom) spines?

If we drop spines and create pseudo 0-spine with ax.axhline we could move divergent values under the "spine". Would probably need some axis limit magic, or duplicate axis so div lines are then always the same

OriolAbril added some commits May 26, 2019

Modify legend
Set color as chain identifier, and make legend with unique elements only
@OriolAbril

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Do we need top, left, right, (bottom) spines?

If we drop spines and create pseudo 0-spine with ax.axhline we could move divergent values under the "spine". Would probably need some axis limit magic, or duplicate axis so div lines are then always the same

I can see that all the divergences in the right column are the same, but I can't follow you with the solution.

@@ -25,6 +25,7 @@ def plot_trace(
rug_kwargs=None,
hist_kwargs=None,
trace_kwargs=None,
max_plots=40,

This comment has been minimized.

Copy link
@OriolAbril

OriolAbril May 26, 2019

Author Contributor

I have set a preliminary maximum here, which will be removed when we decide on the implementation of global variables #609

This comment has been minimized.

Copy link
@aloctavodia

aloctavodia May 30, 2019

Contributor

We should have a warning, telling users wanting to plot more than max_plot plots that only max_plots are actually plotted.

This comment has been minimized.

Copy link
@aloctavodia

aloctavodia May 30, 2019

Contributor

Maybe max_plot should also accepts None, in case users does not want to put a limit

This comment has been minimized.

Copy link
@OriolAbril

OriolAbril May 30, 2019

Author Contributor

I agree on the warning, I will add it as soon as possible. About accepting None, I think it is not necessary, if no limit is wanted, it is easy to set it to 10000 and let matplotlib hanging be the limit.

This comment has been minimized.

Copy link
@OriolAbril

OriolAbril May 30, 2019

Author Contributor

I think it may be better to merge this once it the warging is added, and afterwards do another PR to implement package parameters.

This comment has been minimized.

Copy link
@aloctavodia

aloctavodia May 30, 2019

Contributor

Right about setting something to a large number but is more elegant to have a special keyword for that case. Anyway, I agree we should add the warning and then merge.

This comment has been minimized.

Copy link
@OriolAbril

OriolAbril May 30, 2019

Author Contributor

I added both, None option and warning

@@ -240,3 +291,45 @@ def _histplot_op(ax, data, **kwargs):
bins = get_bins(data)
ax.hist(data, bins=bins, align="left", density=True, **kwargs)
return ax

This comment has been minimized.

Copy link
@OriolAbril

OriolAbril May 30, 2019

Author Contributor

This function should probably be deleted. It looks like kde/hist is handled by plot_dist

This comment has been minimized.

Copy link
@aloctavodia

aloctavodia May 30, 2019

Contributor

good catch!

OriolAbril added some commits May 30, 2019

@aloctavodia aloctavodia merged commit 6f60212 into arviz-devs:master May 30, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+3.2%) to 97.441%
Details
@ColCarroll

This comment has been minimized.

Copy link
Member

commented May 30, 2019

This is great, @OriolAbril, thank you!

@OriolAbril OriolAbril deleted the OriolAbril:traceplot branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.