Add circular histogram and KDE plot #1266

agustinaarroyuelo · 2020-06-25T16:57:02Z

Description

This PR adds circular histogram and circular KDE plot.
Histogram example (matplotlib):

Histogram example (bokeh):

Although with this code you can get a circular KDE, there are a couple of things to improve in the KDE that will result in better behavior in a circular setting. We are planning to follow on Tomas Capretto's work on KDE. You can read the details in his report.

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

arviz/plots/backends/matplotlib/distplot.py

arviz/plots/backends/matplotlib/kdeplot.py

arviz/plots/backends/bokeh/distplot.py

aloctavodia · 2020-06-30T21:20:55Z

arviz/plots/backends/bokeh/distplot.py

+            **hist_kwargs,
+        )
+
+        ticks = np.linspace(-np.pi, np.pi, 9)


This could be

ticks = np.linspace(-np.pi, np.pi, len(labels), endpoint=False)

aloctavodia · 2020-06-30T21:22:41Z

arviz/plots/backends/bokeh/distplot.py

+            y=0,
+            inner_radius=0,
+            outer_radius=np.max(outer_radius) * 1.1,
+            start_angle=ticks[:-1],


and this could just be start_angle=ticks

aloctavodia · 2020-06-30T21:24:50Z

arviz/plots/backends/bokeh/distplot.py

+            inner_radius=0,
+            outer_radius=np.max(outer_radius) * 1.1,
+            start_angle=ticks[:-1],
+            end_angle=ticks[:-1] + 0.002,


do we need the 0.002 or could we just use end_angle=ticks?

agustinaarroyuelo · 2020-07-01T12:35:09Z

Thanks @aloctavodia for your comments!

aloctavodia · 2020-07-01T13:41:43Z

Nitpicking: The grey radial lines indicating the angles are of the same color as the wedge's borders. We may want to 1)remove the borders (make them the same color as the wedges), 2) make the border black, 3) add a small blank space between wedges. Probably the first option is easier and it will work well. But the last one will be more consistent with the matplotlib histogram (circular and not)

fix docstring fix changelog

OriolAbril

Looks great!

I have a couple of high level questions too, the comments are minor nits.

Do both degrees and radians units work with bokeh hist, mpl hist and mpl kde? Do you think it would be worth it to create a function in plot_utils that got is_circular and returned the corresponding ticks and ticklabels?

I understand all polar plots will show the complete circle, do you plan to support other angular ranges? (again, I know close to nothing on circular variables so I am not even sure it would be worth to support half circle or something like this)

CHANGELOG.md

arviz/plots/backends/bokeh/distplot.py

arviz/plots/backends/bokeh/energyplot.py

OriolAbril · 2020-07-01T18:30:59Z

arviz/plots/backends/matplotlib/distplot.py

    if ax is None:
-        ax = plt.gca()
+        ax = plt.gca(polar=is_circular)


Is using gca instead of a new axes intended or a forgotten TODO from when bokeh was introduced? cc @ahartikainen @canyon289 @aloctavodia

I think this is intended

Will gca grab possibly old axis here? Have we created a new figure somewhere?

if present, plt.gca will grab old axes yes

I believe this is considered a "best practice" with matplotlib, and a pattern they endorse.

arviz/plots/distplot.py

arviz/plots/backends/matplotlib/distplot.py

arviz/plots/distplot.py

codecov · 2020-07-01T19:11:19Z

Codecov Report

Merging #1266 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1266      +/-   ##
==========================================
+ Coverage   93.02%   93.12%   +0.09%     
==========================================
  Files         101      101              
  Lines        9908    10034     +126     
==========================================
+ Hits         9217     9344     +127     
+ Misses        691      690       -1

Impacted Files	Coverage Δ
arviz/plots/backends/bokeh/energyplot.py	`100.00% <ø> (ø)`
arviz/plots/distplot.py	`93.33% <ø> (-0.42%)`	⬇️
arviz/plots/backends/bokeh/distplot.py	`87.87% <100.00%> (+2.69%)`	⬆️
arviz/plots/backends/matplotlib/distplot.py	`89.58% <100.00%> (+6.25%)`	⬆️
arviz/plots/backends/matplotlib/kdeplot.py	`97.46% <100.00%> (+0.20%)`	⬆️
arviz/plots/kdeplot.py	`100.00% <100.00%> (ø)`
arviz/plots/plot_utils.py	`94.40% <100.00%> (+0.20%)`	⬆️
arviz/data/io_cmdstanpy.py	`100.00% <0.00%> (ø)`
arviz/plots/backends/matplotlib/forestplot.py	`97.93% <0.00%> (+<0.01%)`	⬆️
arviz/stats/stats_utils.py	`95.66% <0.00%> (+0.01%)`	⬆️
... and 9 more

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 1ef0dd5...583079c. Read the comment docs.

agustinaarroyuelo · 2020-07-03T14:35:03Z

Thanks for your comments @OriolAbril.

Yes, both degrees and radians will work. Creating a function to manage ticks and ticklabels can be specially useful for the circular plots in Bokeh.

I am almost sure that you can get an angular range plot by working on the axes returned by the Matplotlib function. Not quite sure about Bokeh. Since it does not support polar projection, things can get a bit tricky.

arviz/plots/distplot.py

aloctavodia · 2020-07-06T12:38:04Z

arviz/plots/backends/matplotlib/distplot.py

    if ax is None:
-        ax = plt.gca()
+        ax = plt.gca(polar=is_circular)


I think this is intended

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

minor fix

arviz/plots/backends/bokeh/distplot.py

ahartikainen · 2020-07-06T20:08:43Z

arviz/plots/backends/bokeh/distplot.py

+
+        if is_circular == "degrees":
+            edges = np.deg2rad(edges)
+            labels = ["0°", "45°", "90°", "135°", "180°", "225°", "270°", "315°"]


We are always showing a full circle plot?

I think that's what everybody does (I know this is not a very good argument, haha)

ahartikainen · 2020-07-06T20:13:39Z

arviz/plots/backends/matplotlib/distplot.py

    if ax is None:
-        ax = plt.gca()
+        ax = plt.gca(polar=is_circular)


Will gca grab possibly old axis here? Have we created a new figure somewhere?

Co-authored-by: Ari Hartikainen <ahartikainen@users.noreply.github.com>

aloctavodia · 2020-07-10T14:10:02Z

I think this is ready to merge, right? @ahartikainen @OriolAbril what do you think?

ColCarroll

this is really clean code!

ColCarroll · 2020-07-10T21:32:17Z

arviz/plots/backends/matplotlib/distplot.py

    if ax is None:
-        ax = plt.gca()
+        ax = plt.gca(polar=is_circular)


I believe this is considered a "best practice" with matplotlib, and a pattern they endorse.

ColCarroll · 2020-07-10T21:34:16Z

Also, does this make it easy to recreate MPL's logo?

OriolAbril · 2020-07-10T22:52:35Z

arviz/plots/backends/matplotlib/distplot.py

+    if rotated or is_circular:
        ax.set_yticks(bins[:-1])


In polar plots, isn't the y axis the radial one? could this be setting the radial ticks with x values?

thanks for spotting this!

arviz/plots/backends/matplotlib/distplot.py

arviz/plots/kdeplot.py

OriolAbril · 2020-07-10T22:59:43Z

arviz/tests/base_tests/test_plot_utils.py

+# Skip tests if bokeh not installed
+bkp = importorskip("bokeh.plotting")  # pylint: disable=invalid-name


I think this will skip all tests in the file if bokeh is not installed. Maybe we should create a test_plot_utils_bokeh? similarly to what is done with test_stats and test_utils?

Òr maybe a regular skipif? something similar to https://github.com/arviz-devs/arviz/blob/master/arviz/tests/external_tests/test_data_pystan.py#L22

I used skipif, but I am not sure about the import statement placement. Let me know what you think

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

OriolAbril · 2020-07-11T15:56:15Z

arviz/plots/backends/matplotlib/distplot.py

-    else:
-        ax.set_xticks(bins[:-1])


I think this should not be removed as it will modify the behaviour of the plot, if it has to be skipped for circular plots I'd suggest a elif not is_circular

arviz/tests/base_tests/test_plot_utils.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

OriolAbril · 2020-07-11T19:02:05Z

arviz/tests/base_tests/test_plots_matplotlib.py

+        {"is_circular": False},
+        {"is_circular": True},
+        {"is_circular": "radians"},
+        {"is_circular": "degrees"},
+    ],
+)
 def test_plot_dist(continuous_model, kwargs):
    axes = plot_dist(continuous_model["x"], **kwargs)


I was quite shocked by the low coverage given that there are tests for everything, I was expecting something close to 100% diff coverage. I think the issue (and same happens with bokeh) is that the variables in the model are continuous, so the kde is plotted. I am not sure if it would be better to add a new test of divide the fixture into two so all combinations are tested

OriolAbril · 2020-07-13T14:21:56Z

Nice! Now everything checks out, thanks! I think it's ready to merge

aloctavodia reviewed Jun 25, 2020

View reviewed changes

arviz/plots/backends/matplotlib/distplot.py Outdated Show resolved Hide resolved

arviz/plots/backends/matplotlib/distplot.py Outdated Show resolved Hide resolved

arviz/plots/backends/matplotlib/kdeplot.py Outdated Show resolved Hide resolved

aloctavodia reviewed Jun 29, 2020

View reviewed changes

arviz/plots/backends/bokeh/distplot.py Outdated Show resolved Hide resolved

arviz/plots/backends/bokeh/distplot.py Outdated Show resolved Hide resolved

arviz/plots/backends/bokeh/distplot.py Outdated Show resolved Hide resolved

aloctavodia reviewed Jun 30, 2020

View reviewed changes

agustinaarroyuelo added 5 commits July 1, 2020 11:53

add circular histogram and KDE plot

6dc14ca

add circular histogram using bokeh

f82a765

fix bin computation

dab16c1

fix overlapping bins

f811169

update changelog

599825c

agustinaarroyuelo changed the title ~~[WIP] Add circular histogram and KDE plot~~ Add circular histogram and KDE plot Jul 1, 2020

fix errors on plot_energy

b1be37c

fix docstring fix changelog

OriolAbril reviewed Jul 1, 2020

View reviewed changes

OriolAbril reviewed Jul 3, 2020

View reviewed changes

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

aloctavodia approved these changes Jul 6, 2020

View reviewed changes

agustinaarroyuelo and others added 7 commits July 6, 2020 11:34

update CHANGELOG.md

8f02d64

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

several fixes

2f5b77d

minor fix

define tick placement function

ea8859f

update docstring

e8db4dd

update docstring

f41056f

update tests mpl

680f36f

run black

506048e

ahartikainen reviewed Jul 6, 2020

View reviewed changes

agustinaarroyuelo and others added 5 commits July 7, 2020 08:28

update hist_kwargs

5793399

Co-authored-by: Ari Hartikainen <ahartikainen@users.noreply.github.com>

add test for plot_utils function

6020607

use importorskip for testing bokeh function

3c7670a

fix test

3c42a3a

align mpl histogram bars to center

5b0a0a4

fix bokeh wedge alignment

8a99512

ahartikainen approved these changes Jul 10, 2020

View reviewed changes

ColCarroll reviewed Jul 10, 2020

View reviewed changes

OriolAbril approved these changes Jul 10, 2020

View reviewed changes

agustinaarroyuelo and others added 4 commits July 11, 2020 11:00

update ylim

5bf3064

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

update kdeplot docstring

97cdaca

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

fix yticks

4fd0f5c

add skipif in test_plot_utils

42aa26a

OriolAbril reviewed Jul 11, 2020

View reviewed changes

agustinaarroyuelo and others added 2 commits July 11, 2020 15:00

update docstring

d97e1e0

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

change yticks

b6e50f1

OriolAbril reviewed Jul 11, 2020

View reviewed changes

update tests

583079c

aloctavodia merged commit 04201c5 into arviz-devs:master Jul 14, 2020

agustinaarroyuelo deleted the circular_histogram branch July 14, 2020 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add circular histogram and KDE plot #1266

Add circular histogram and KDE plot #1266

agustinaarroyuelo commented Jun 25, 2020 •

edited

Loading

aloctavodia Jun 30, 2020

aloctavodia Jun 30, 2020

aloctavodia Jun 30, 2020

agustinaarroyuelo commented Jul 1, 2020

aloctavodia commented Jul 1, 2020

OriolAbril left a comment

OriolAbril Jul 1, 2020

aloctavodia Jul 6, 2020

ahartikainen Jul 6, 2020

OriolAbril Jul 7, 2020

ColCarroll Jul 10, 2020

codecov bot commented Jul 1, 2020 •

edited

Loading

agustinaarroyuelo commented Jul 3, 2020

aloctavodia Jul 6, 2020

ahartikainen Jul 6, 2020

aloctavodia Jul 6, 2020

ahartikainen Jul 6, 2020

aloctavodia commented Jul 10, 2020

ColCarroll left a comment

ColCarroll Jul 10, 2020

ColCarroll commented Jul 10, 2020

OriolAbril Jul 10, 2020

agustinaarroyuelo Jul 11, 2020

OriolAbril Jul 10, 2020

OriolAbril Jul 10, 2020

agustinaarroyuelo Jul 11, 2020

OriolAbril Jul 11, 2020

OriolAbril Jul 11, 2020

OriolAbril commented Jul 13, 2020

		# Skip tests if bokeh not installed
		bkp = importorskip("bokeh.plotting") # pylint: disable=invalid-name

Add circular histogram and KDE plot #1266

Add circular histogram and KDE plot #1266

Conversation

agustinaarroyuelo commented Jun 25, 2020 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agustinaarroyuelo commented Jul 1, 2020

aloctavodia commented Jul 1, 2020

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

codecov bot commented Jul 1, 2020 • edited Loading

Codecov Report

agustinaarroyuelo commented Jul 3, 2020

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

aloctavodia commented Jul 10, 2020

ColCarroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColCarroll commented Jul 10, 2020

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

Choose a reason for hiding this comment

OriolAbril commented Jul 13, 2020

agustinaarroyuelo commented Jun 25, 2020 •

edited

Loading

codecov bot commented Jul 1, 2020 •

edited

Loading