[WIP] Histogram support addition to distplot.py #47

imperorrp · 2024-04-15T19:42:27Z

This PR aims to add support for histograms in distplot.py (Issue #24 ), allowing marginal densities to be visualized in this new form, apart from preexisting KDE and ECDF forms

📚 Documentation preview 📚: https://arviz-plots--47.org.readthedocs.build/en/47/

imperorrp · 2024-04-20T23:22:47Z

@OriolAbril I had to dig a bit deeper into understanding how the plotting actually works as coded, but I think I've managed to get step 1 (computing the histogram) roughly done- I'll probably have to combine these DataArrays into a Dataset and then create the visual element for mapping. Also, I'll remove the print statements I've added in later commits- just added them to help with seeing the outputs. Is something like this alright?- Looping through the data variables in distribution to get DataArrays and then passing that plus the dimensions to be reduced to the xarray_einstats.histogram() function.

Also, I was going through the flow of how density was computed for KDE as an example (in Arviz-Stats). I saw bandwidth computing was done for KDE- but that won't be necessary for plotting histograms, right?

imperorrp · 2024-04-22T12:45:45Z

Combined the xarray_einstats.numba.histogram produced DataArrays into a Dataset:

(This is with the Schools dataset)

It's in the style of the KDE dataset returned by distribution.azstats.kde to density

imperorrp · 2024-04-22T15:19:29Z

Changed the plot_axis coords to 'x' and 'y' in place of 'bin_midpoint' and 'bin_height':

Also added a visual element function, backend interface and matplotlib implementation with matplotlib.pyplot.bar:

The bar widths are different due to the different scales of the x-axis. I'm not sure how this can be fixed yet if keeping the width a consistent value is desired though. Also, the number of bins is set to 20 by default. Some bins have too few values to be visible as their respective bars are too low in height compared to the taller ones.

OriolAbril

It is looking very good already. The computation of the histogram is a bit trickier and would not be part of the gsoc project, but I think it will be helpful to get you more familiar with xarray.

I have tried to focus on the plotting and contributing workflow with the comments. Let me know if you have any doubts related to testing.

OriolAbril · 2024-04-22T17:57:46Z

src/arviz_plots/backend/__init__.py

@@ -99,6 +99,29 @@ def create_plotting_grid(


 # "geoms"
+def hist(x, height, target, *, width=1.6, bottom=None, align="center", color=None):


given this is a histogram instead of a bar plot, I would require both left and right bin edges and use those for the bars, therefore removing both width and align parameter. We might consider something like rwidth like matplotlib (relative width) with a default to 1 in case we didn't want bins to be draws with their actual width, but I would leave that for later (if at all).

This will require some changes to the data restructure utility function but I am not too worried about this, at some point we'll have a function in arviz-stats to take care of all this.

OriolAbril · 2024-04-22T18:07:22Z

src/arviz_plots/plots/distplot.py

+
+                # number of bins is set to 20 by default
+                hist = histogram(
+                    da=var_data, dims=density_dims, bins=20, **stats_kwargs.get("density", {})


the default number of bins should be taken care of by histogram function. Moreover, if we believed a different default were needed, it would have to happen via stats_kwargs and setdefault so a user could modify that value.

Just modified this in the latest commit to do so

Now by default, with no bins provided (azp.plot_dist(schools, kind='hist')) and the same example dataset, this is produced:

And with bins=40 provided by a user (azp.plot_dist(schools, kind='hist', stats_kwargs={"density": {"bins":40}})), this is produced:

Will look at your other comments and make changes for those too now

OriolAbril · 2024-04-22T18:17:22Z

src/arviz_plots/backend/__init__.py

+def hist(x, height, target, *, width=1.6, bottom=None, align="center", color=None):
+    """Interface for a hist plot.
+
+    Parameters


We haven't been completely consistent adding the docstrings to this module, but the idea is for the documentation here so define the behaviour all backends have to comply with. It also isn't a general plotting backend interface but a common interface for our own plots to use which means we can be quite restrictive about the features.

For example, I would only accept array_like for x, height, width... I don't think the extra edge case of floats would be used anywhere. It also means the descriptions should not depend on any backend.

OriolAbril · 2024-04-22T18:22:01Z

src/arviz_plots/backend/__init__.py

+    color : color or array-like, optional
+        The colors of the bar faces.


I might start with the same color for all rectangles, mostly because I am not sure how easy it will be to support the different color rectangles in all backends. If we then see it is feasible and of interest we can add that extra case.

There should also be a facecolor and an edgecolor arguments given there is a color argument and the generated form is "fillable" to match the expectations from https://arviz-plots.readthedocs.io/en/latest/api/backend/index.html#backend-interface-arguments

imperorrp · 2024-04-26T20:28:34Z

It is looking very good already. The computation of the histogram is a bit trickier and would not be part of the gsoc project, but I think it will be helpful to get you more familiar with xarray.

I have tried to focus on the plotting and contributing workflow with the comments. Let me know if you have any doubts related to testing.

Thank you, I will take a look at your reviews and advice!

imperorrp · 2024-05-28T22:03:05Z

@OriolAbril Latest commit has changes incorporating advice from your comments. Modified data restructure utility function to keep left and right edges info now until the matplotlib backend calculates midpoints from these at the end to create the bars, and I've updated the docstring and color/facecolor/edgecolor interface

OriolAbril

Left a comment on how to use matplotlib bar function to generate histogram looking plot. Let me know if you need any help to generate the bokeh equivalent

src/arviz_plots/backend/matplotlib/__init__.py

OriolAbril · 2024-06-22T16:36:49Z

By the way, I commented on the ess and mcse issues but forgot to comment here. There is a histogram function available in arviz-stats now. I think the output will have a similar (if not the same) shape to what you are using

imperorrp · 2024-07-05T14:13:16Z

By the way, I commented on the ess and mcse issues but forgot to comment here. There is a histogram function available in arviz-stats now. I think the output will have a similar (if not the same) shape to what you are using

Altered the code to make use of this function. Also, it seems like point estimate and credible intervals are being plotted so close to the x axis for histogram plots because the default 'y' aesthetic value/s returned by plot collection are pretty low in relation. The maximum y-axis values for the KDE/ECDF plots are way lower so it's not a problem for them. I initially thought there was some issue with this aesthetic mapping when type hist is picked, but it works fine- it's just really small values compared to the histogram heights. Should we mention this in the documentation, asking users to define custom 'y' aesthetic values (in pc_kwargs)when using kind=histogram for better visibility?

For the plot_dist example where one subplot is created for each variable:

azp.plot_dist(
        data,
        kind="kde",
        pc_kwargs={
            "cols": ["__variable__"],
            "aes": {"color": ["school"], "y": ["school"]},
            "y": np.linspace(0, 0.06, 9),
        },
        aes_map={
            "kde": ["color"],
            "point_estimate": ["color", "y"],
            "credible_interval": ["y"],
        },
    )
    plt.savefig("distplot-kde-cols-matplotlib.png")

Point estimates when switching from kind="kde" to kind="hist":

azp.plot_dist(
        data,
        kind="hist",
        pc_kwargs={
            "cols": ["__variable__"],
            "aes": {"color": ["school"], "y": ["school"]},
            "y": np.linspace(0, 0.06, 9),
        },
        aes_map={
            "kde": ["color"],
            "point_estimate": ["color", "y"],
            "credible_interval": ["y"],
        },
    )
    plt.savefig("distplot-hist-cols-matplotlib.png")

This is easily fixable with editing the np.linspace line though-

"y": np.linspace(0, 600, 9),

We could maybe add this in the documentation so users know to keep this in mind.

OriolAbril · 2024-07-06T03:44:53Z

src/arviz_plots/plots/distplot.py

+            density = distribution.azstats.histogram(
+                dims=density_dims, **stats_kwargs.get("density", {})
+            )


we should pass density=True as default here. This will normalize the histogram so its area is 1, and will therefore generate histograms with heights similar to those of the kde. That will should take care of the non-reusability of the overlayed example when changing hist/kde. For ecdf/dotplots it will probably break too, but I don't think it matters, ecdf always goes 0-1 whereas dotplots are particularly strange, they should not be expected to act as drop-in replacements of either kde nor hist.

Yeah adding that seems to work for the overlay example, without having to alter the np.linspace parameters.

OriolAbril

As always, go over all comments before starting to address any single one of them

OriolAbril · 2024-07-18T13:26:04Z

src/arviz_plots/plots/distplot.py

@@ -97,6 +99,7 @@ def plot_dist(

          * "kde" -> passed to :func:`~arviz_plots.visuals.line_xy`
          * "ecdf" -> passed to :func:`~arviz_plots.visuals.ecdf_line`
+          * "hist" -> passed to :func: `~WIP`


update this and add the new visual function to https://github.com/arviz-devs/arviz-plots/blob/main/docs/source/api/visuals.rst

OriolAbril · 2024-07-18T13:27:21Z

src/arviz_plots/plots/distplot.py

@@ -250,6 +253,16 @@ def plot_dist(
                **density_kwargs,
            )

+        elif kind == "hist":
+            density = distribution.azstats.histogram(
+                dims=density_dims, density=True, **stats_kwargs.get("density", {})


density=True should be added to stats_kwargs with setdefault. Then, if a user wanted to they can set it to False, keep the axis and use the y axis to look at counts for example. The task itself isn't really important, what is is that it should be possible

OriolAbril · 2024-07-18T13:30:42Z

src/arviz_plots/plots/distplot.py

@@ -352,7 +374,7 @@ def plot_dist(
            labeller=labeller,
            **title_kwargs,
        )
-    if (kind == "kde") and (plot_kwargs.get("remove_axis", True) is not False):
+    if (kind in ("kde", "hist")) and (plot_kwargs.get("remove_axis", True) is not False):


I would modify this logic. Instead, I would set the value of the remove_axis depending on kind at the top, right after copying plot_kwargs.

if kind in ("hist", "ecdf"): plot_kwargs.setdefault("remove_axis", False)

I think as a default we'll want to remove the axis for kde and quantile dotplot but keep it for the others.

OriolAbril · 2024-07-18T13:31:46Z

src/arviz_plots/visuals/__init__.py

@@ -13,6 +13,21 @@
 from arviz_base.labels import BaseLabeller


+def plot_hist(da, target, backend, **kwargs):


I would use hist only to "reserve" the plot_xyz for batteries included functions, otherwise it might get confusing.

OriolAbril · 2024-07-18T13:34:09Z

src/arviz_plots/backend/__init__.py

@@ -99,6 +99,33 @@ def create_plotting_grid(


 # "geoms"
+def hist(y, l_e, r_e, target, *, bottom=None, color=None, facecolor=None, edgecolor=None):


I went over all potential keywords in https://arviz-plots.readthedocs.io/en/latest/api/backend/index.html#backend-interface-arguments and alpha should be added here too.

imperorrp · 2024-07-20T17:22:23Z

Just pushed changes per last review

tests/test_plots.py

codecov-commenter · 2024-07-23T11:20:08Z

Codecov Report

Attention: Patch coverage is 57.81250% with 27 lines in your changes missing coverage. Please review.

Project coverage is 84.84%. Comparing base (1298e42) to head (2304364).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/arviz_plots/plots/utils.py	13.33%	13 Missing ⚠️
src/arviz_plots/backend/none/__init__.py	58.33%	5 Missing ⚠️
src/arviz_plots/backend/bokeh/__init__.py	55.55%	4 Missing ⚠️
src/arviz_plots/backend/matplotlib/__init__.py	63.63%	4 Missing ⚠️
src/arviz_plots/backend/__init__.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   85.85%   84.84%   -1.02%     
==========================================
  Files          17       17              
  Lines        1951     2012      +61     
==========================================
+ Hits         1675     1707      +32     
- Misses        276      305      +29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OriolAbril · 2024-07-23T12:30:54Z

src/arviz_plots/backend/bokeh/__init__.py

+    **artist_kws,
+):
+    """Interface to Bokeh for a histogram bar plot."""
+    artist_kws.setdefault("level", "glyph")


is this necessary?

OriolAbril · 2024-07-23T12:35:47Z

src/arviz_plots/plots/utils.py

+
+
+# WIP: dropping bin_midpoints calculation and keeping bin_edges
+def restructure_hist_data(hist_dict):


this can be removed

OriolAbril · 2024-07-25T17:45:47Z

This will need to be rebased and double checked to make it compatible with plotly. Take care of these last two comments left and after that I'll rebase and add more commits to the PR so it works with plotly

…visual element function, backend interface and matplotlib backend function for plotting histogram

…mber of bins

…ta and modified docstrings for hist backend interface

…ted hypothesis time limit to 2 seconds and modified backend hist plotting function

…ious commits and updated restructure_hist_data() docstring

…ual element slightly for new returned hist density data structure

…by default for histograms

…ization from test_plot_dist_models

…ing functions, renamed 'plot_hist' to 'hist', modified `remove_axis` logic slightly and set density=True as default in stats_kwargs

It needs a fix in arviz-stats to work

…ith updates fromrootogram plot

OriolAbril reviewed Apr 22, 2024

View reviewed changes

OriolAbril reviewed Jun 5, 2024

View reviewed changes

src/arviz_plots/backend/matplotlib/__init__.py Outdated Show resolved Hide resolved

OriolAbril reviewed Jul 6, 2024

View reviewed changes

OriolAbril force-pushed the plot_dist-histogram branch 2 times, most recently from 98e5d1f to 8f43767 Compare July 18, 2024 13:21

OriolAbril reviewed Jul 18, 2024

View reviewed changes

OriolAbril reviewed Jul 23, 2024

View reviewed changes

tests/test_plots.py Outdated Show resolved Hide resolved

OriolAbril reviewed Jul 23, 2024

View reviewed changes

imperorrp mentioned this pull request Aug 12, 2024

Adding New Rootogram Plot #81

Closed

imperorrp added 11 commits August 13, 2024 14:51

WIP histogram addition to distplot.py

fc11948

Added histogram computing if kind='hist'

da5889e

reformatted histogram dataarrays into a dataset

fb1bbb2

Modified histogram dataset plot_axis coords to 'x' and 'y' and added …

b463042

…visual element function, backend interface and matplotlib backend function for plotting histogram

Allowing xarray_einstats.histogram() function to determine default nu…

525b655

…mber of bins

Modified histogram data restructuring function to include bin edge da…

ece9e62

…ta and modified docstrings for hist backend interface

added plot_dist test parametrizations for kind=kde, hist, ecdf, adjus…

bbee8f7

…ted hypothesis time limit to 2 seconds and modified backend hist plotting function

added width to histogram plotting, removed print statements from prev…

60df91a

…ious commits and updated restructure_hist_data() docstring

switched histogram computation to arviz stats, modified plot_hist vis…

67838ad

…ual element slightly for new returned hist density data structure

added 'density=True' to normalize histogram heights and removed axis …

327334d

…by default for histograms

added Bokeh backend for hist visual element and removed ecdf parametr…

23af242

…ization from test_plot_dist_models

imperorrp and others added 6 commits August 13, 2024 14:51

updated docstring, added 'alpha' argument to the 'hist' backend plott…

442dc5e

…ing functions, renamed 'plot_hist' to 'hist', modified `remove_axis` logic slightly and set density=True as default in stats_kwargs

Added 'hist' to visuals.rst

8256833

deactivate tests for hist kind and multiple models

0ea7dd2

It needs a fix in arviz-stats to work

removed restructure_dist and glyph default artist kwarg in bokeh

8117fde

updated hist backend interface and matplotlib hist backend function w…

b7531a9

…ith updates fromrootogram plot

plotly hist and plot_dist improvements

f7a56c6

OriolAbril force-pushed the plot_dist-histogram branch from 2bd3519 to f7a56c6 Compare August 13, 2024 23:12

OriolAbril merged commit a1e32a5 into arviz-devs:main Aug 13, 2024
4 checks passed

OriolAbril mentioned this pull request Oct 18, 2024

Set default color in plot_dist #56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Histogram support addition to distplot.py #47

[WIP] Histogram support addition to distplot.py #47

imperorrp commented Apr 15, 2024 •

edited by github-actions bot

Loading

imperorrp commented Apr 20, 2024

imperorrp commented Apr 22, 2024

imperorrp commented Apr 22, 2024 •

edited

Loading

OriolAbril left a comment

OriolAbril Apr 22, 2024

OriolAbril Apr 22, 2024

OriolAbril Apr 22, 2024

imperorrp May 27, 2024 •

edited

Loading

OriolAbril Apr 22, 2024

OriolAbril Apr 22, 2024

imperorrp commented Apr 26, 2024

imperorrp commented May 28, 2024

OriolAbril left a comment

OriolAbril commented Jun 22, 2024

imperorrp commented Jul 5, 2024

OriolAbril Jul 6, 2024

imperorrp Jul 7, 2024

OriolAbril left a comment

OriolAbril Jul 18, 2024

OriolAbril Jul 18, 2024

OriolAbril Jul 18, 2024

OriolAbril Jul 18, 2024

OriolAbril Jul 18, 2024

imperorrp commented Jul 20, 2024

codecov-commenter commented Jul 23, 2024

OriolAbril Jul 23, 2024

OriolAbril Jul 23, 2024

OriolAbril commented Jul 25, 2024

		@@ -99,6 +99,29 @@ def create_plotting_grid(


		# "geoms"
		def hist(x, height, target, *, width=1.6, bottom=None, align="center", color=None):

		color : color or array-like, optional
		The colors of the bar faces.

		@@ -13,6 +13,21 @@
		from arviz_base.labels import BaseLabeller


		def plot_hist(da, target, backend, **kwargs):

		@@ -99,6 +99,33 @@ def create_plotting_grid(


		# "geoms"
		def hist(y, l_e, r_e, target, *, bottom=None, color=None, facecolor=None, edgecolor=None):



		# WIP: dropping bin_midpoints calculation and keeping bin_edges
		def restructure_hist_data(hist_dict):

[WIP] Histogram support addition to distplot.py #47

[WIP] Histogram support addition to distplot.py #47

Conversation

imperorrp commented Apr 15, 2024 • edited by github-actions bot Loading

imperorrp commented Apr 20, 2024

imperorrp commented Apr 22, 2024

imperorrp commented Apr 22, 2024 • edited Loading

OriolAbril left a 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

imperorrp May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imperorrp commented Apr 26, 2024

imperorrp commented May 28, 2024

OriolAbril left a comment

Choose a reason for hiding this comment

OriolAbril commented Jun 22, 2024

imperorrp commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imperorrp commented Jul 20, 2024

codecov-commenter commented Jul 23, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril commented Jul 25, 2024

imperorrp commented Apr 15, 2024 •

edited by github-actions bot

Loading

imperorrp commented Apr 22, 2024 •

edited

Loading

imperorrp May 27, 2024 •

edited

Loading