Adding Plot ESS Evolution #71
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 85.65% 86.47% +0.81%
==========================================
Files 22 23 +1
Lines 2545 2736 +191
==========================================
+ Hits 2180 2366 +186
- Misses 365 370 +5 ☔ View full report in Codecov by Sentry. |
| Valid keys are: | ||
| * "bulk" -> passed to :func:`~arviz_plots.visuals.scatter_xy` and | ||
| :func:`~arviz_plots.visuals.line_xy` | ||
| * "tail" -> passed to :func:`~arviz_plots.visuals.scatter_xy` and | ||
| :func:`~arviz_plots.visuals.line_xy` | ||
| * divergence -> passed to :func:`~.visuals.trace_rug` | ||
| * title -> passed to :func:`~arviz_plots.visuals.labelled_title` | ||
| * label -> passed to :func:`~arviz_plots.visuals.labelled_x` and | ||
| :func:`~arviz_plots.visuals.labelled_y` | ||
| * mean -> passed to WIP | ||
| * sd -> passed to WIP |
There was a problem hiding this comment.
lists in rST need an empty line before and after to be rendered correctly.
| Passed to :class:`arviz_plots.PlotCollection.wrap` | ||
| Returns |
There was a problem hiding this comment.
| Passed to :class:`arviz_plots.PlotCollection.wrap` | |
| Returns | |
| Passed to :class:`arviz_plots.PlotCollection.wrap` | |
| Returns |
this also needs a white line to separate the sections
| x_diff = 1 / n_points | ||
| if "x" not in pc_kwargs: | ||
| pc_kwargs["x"] = np.linspace(-x_diff / 3, x_diff / 3, distribution.sizes["model"]) |
There was a problem hiding this comment.
| x_diff = 1 / n_points | |
| if "x" not in pc_kwargs: | |
| pc_kwargs["x"] = np.linspace(-x_diff / 3, x_diff / 3, distribution.sizes["model"]) | |
| n_models = distribution.sizes["model"] | |
| x_diff = min(1 / n_points / 3, 1 / n_points * n_models / 10) | |
| pc_kwargs.setdefault("x", np.linspace(-x_diff, x_diff, n_models)) |
Seeing the plot now, 1/3 would probably look good for several models, but for only 2 of them the points end up equispaced (the model1 point corresponding to quantile 0 is as far from the model0 point corresponding to quantile 0 than from the model0 point corresponding to quantile 0.05). This change will result in smaller spacing with small number of models, but never going over 1/3 of the available space.
There was a problem hiding this comment.
I thought I was reviewing the plot_ess PR. Having x with an aesthetic mapping here makes no sense and should be completely removed.
| aes_map.setdefault("mean", plot_collection.aes_set) | ||
| aes_map.setdefault("mean", ["linestyle", "-"]) | ||
| aes_map.setdefault("sd", plot_collection.aes_set) | ||
| aes_map.setdefault("sd", ["linestyle", "-"]) |
There was a problem hiding this comment.
I think by default we don't want any aesthetic mapping to be applied to either mean nor sd, so I would remove these 4 lines. Also note the lines 2 and 4 are not doing anything because the lines right before already define that key, but if they did, it would be incorrect. Values in the aes_map dict should be sets of strings that represent aesthetics.
This seems a confusion with setting a default linestyle in plot_kwargs. If you want an aesthetic property to always have the same value, use plot_kwargs if instead you want it to depend on a dimension/variable use aes in pc_kwargs to define the mapping and aes_map to indicate to which visual elements should the mapping be applied too.
There is nothing about this concepts in https://arviz-plots.readthedocs.io/en/latest/contributing/new_plot.html because that doc is for writing plots and these are usage questions which are therefore out of scope. The usage docs are instead https://arviz-plots.readthedocs.io/en/latest/tutorials/plots_intro.html and to a much lesser extent https://arviz-plots.readthedocs.io/en/latest/tutorials/intro_to_plotcollection.html
| :func:`~arviz_plots.visuals.line_xy` | ||
| * "tail" -> passed to :func:`~arviz_plots.visuals.scatter_xy` and | ||
| :func:`~arviz_plots.visuals.line_xy` | ||
| * divergence -> passed to :func:`~.visuals.trace_rug` |
There was a problem hiding this comment.
This key should be "rug" instead. The most common thing to use (and therefore default for rug_kind are divergences) but they are not the only option. As we agreed on smaller set of fixed keys in #66, I think this key should be "rug" instead of rug_kind.
Note this will require multiple changes in the function code
| if rug: | ||
| sample_stats = get_group(dt, "sample_stats", allow_missing=True) | ||
| divergence_kwargs = copy(plot_kwargs.get("divergence", {})) | ||
| if ( | ||
| sample_stats is not None | ||
| and "diverging" in sample_stats.data_vars | ||
| and np.any(sample_stats[rug_kind]) # 'diverging' by default | ||
| and divergence_kwargs is not False | ||
| ): |
There was a problem hiding this comment.
We should add a check like in plot_forest to raise an error if plot_kwargs["rug"] is False given there is a top level kwarg for it
| and "diverging" in sample_stats.data_vars | ||
| and np.any(sample_stats[rug_kind]) # 'diverging' by default |
| # if "width" not in div_aes: # should this be hardcoded? | ||
| # divergence_kwargs.setdefault("width", linewidth) |
There was a problem hiding this comment.
Not sure, you'll have to check both backends behaviour. I seem to remember in some cases the default was 0 which meant nothing got plotted, if that were the case then we'd need a default here (which you can get the same way you get the default color).
There was a problem hiding this comment.
I tried this with bokeh, seems like there definitely is some default that isn't zero when no linewidth is used-
There was a problem hiding this comment.
then no need to set it, you can delete these lines
| ylabel = ylabel.format("Relative ESS") | ||
| else: | ||
| ylabel = ylabel.format("ESS") | ||
| xlabel = "Total Number of Draws" |
There was a problem hiding this comment.
| xlabel = "Total Number of Draws" | |
| xlabel = sample_dims[0] if len(sample_dims) == 1 else "Total Number of Draws" |
Plus, I would add that into label_kwargs as text key so it can be modified if desired. Consequently a single "label" key can be used anymore, otherwise it would be impossible to set different names
There was a problem hiding this comment.
So "xlabel", "ylabel" as two separate keys, right?
| # plot x and y axis labels | ||
| # Add varnames as x and y labels | ||
| _, labels_aes, labels_ignore = filter_aes(plot_collection, aes_map, "label", sample_dims) | ||
| label_kwargs = plot_kwargs.get("label", {}).copy() |
There was a problem hiding this comment.
labels also need the "False" value to be available and have an effect
|
Updated plot_ess_evolution. The points and lines for both 'bulk' and 'tail' methods can now be turned on and off via Also added a common ess_dataset computing function since it can be called upto 4 times (one for each of the cartesian product of 'bulk', 'tail' and points, lines). The rug plot element is still WIP. The outputs without the rug are now like:
An aesthetics issue with the color coding of the mean/sd plot elements:For the mean/sd plotting (and this'd similarly apply to plot_ess too) it looks like setting mean and sd to one color by default as I'm doing currently makes it hard to interpret that they have been overlapped when they coincide: When they're not set to the same color, it's visible that they coincide and are overlapped but then the colors might be confusing as they correspond to the 'bulk' and 'tail' plotting elements' colors too: |
imperorrp
force-pushed
the
evolutionplot
branch
from
9e9f9b3
to
4aef204
Compare
|
Rebased plot ess evolution commits |
|
The mean and sd lines are now annotated:
Should the min_ess line also be annotated? (This also applies for ESS plot #58 ). Current Arviz ESS plot doesn't have it annotated (https://github.com/arviz-devs/arviz/blob/main/arviz/plots/backends/matplotlib/essplot.py#L150) but it would probably be nice to have and is easy to add. |
imperorrp
force-pushed
the
evolutionplot
branch
from
28309a4
to
5be8669
Compare
|
Fixed hypothesis tests errors and documentation build |
imperorrp
force-pushed
the
evolutionplot
branch
from
620ae63
to
5743ccc
Compare
updated plot_ess_evolution including a common ess_dataset computing func added mean and sd annotations like essplot docs and example gallery for plot_ess_evolution updated verticalalign logic for mean/sd and correct (although overlaid and not flattened yet) rug is now displayed removed rug plot added tests updated scatter_xy func to plot_ess version fixed docstring altered store_artist for xlabel, ylabel and modified hypothesis tests shifted mean_ess, sd_edd computing to before plot_kwargs check+artist plotting logic updated docstring, added figsizing and set vertical_align for mean and sd text kwargs as setdefault removed 'rankdata' branch of arviz-stats from dependencies docstring typo fix gallery-generator updated for documentation building
OriolAbril
force-pushed
the
evolutionplot
branch
from
7a143c9
to
8416b4a
Compare
First commit for
plot_ess_evolution(), as a branched off plot from legacyplot_essfor kind = 'evolution' (#5). Theline_xyandscatter_xyvisual elements were used for the plotting, with rug plot, axes labelling and title labelling functionality taken from ESS plot functionality. Since the ESS plot PR hasn't been merged yet, thescatter_xyvisual element was duplicated for use here.WIP:
Marking the 'bulk' and 'tail' artists with a legend
Current plot outputs:
azp.plot_ess_evolution(data)azp.plot_ess_evolution(data, var_names=["mu", "tau"])📚 Documentation preview 📚: https://arviz-plots--71.org.readthedocs.build/en/71/