Solve issue #992 : Integrate point_estimate with rcParam #994

percygautam · 2020-01-13T20:37:53Z

Fixes #992 . The following changes are made:

Added "auto" as default-type for point_estimate param for densityplot.py and posteriorplot.py
Added the docstring for changes.
Passes the pytest.

OriolAbril

This looks great! I have just realized that the "mode" option is not yet available in plot_density (see its matplotlib file) it is only available in plot_posterior (see its matplotlib file).

If you feel up for the challenge I can guide you to add the "mode" option to plot_density, otherwise we can merge this and file a new issue.

OriolAbril · 2020-01-13T20:56:10Z

arviz/plots/densityplot.py

- Plot point estimate per variable. Values should be 'mean', 'median' or None.
- Defaults to 'mean'.
+ Plot point estimate per variable. Values should be 'mean', 'median', 'mode' or None.
+ Defaults to 'auto'.


I would explain that auto falls back to the default set in rcParams, otherwise it may be confusing to new users.

Okay, makes sense. I'll add it right away.

OriolAbril · 2020-01-13T20:57:05Z

arviz/plots/posteriorplot.py

@@ -59,7 +60,7 @@ def plot_posterior(
 round_to : int, optional
 Controls formatting of floats. Defaults to 2 or the integer part, whichever is bigger.
 point_estimate: str
- Must be in ('mode', 'mean', 'median', None)
+ Must be in ('mode', 'mean', 'median', None). Defaults to 'auto'.


Same comment as above.

OriolAbril · 2020-01-13T20:58:45Z

arviz/tests/test_plots_matplotlib.py

@@ -657,7 +657,7 @@ def test_plot_rank(models, kwargs):
 {"rope": {"mu": [{"rope": (-2, 2)}], "theta": [{"school": "Choate", "rope": (2, 4)}]}},
 {"point_estimate": "mode"},
 {"point_estimate": "median"},
- {"point_estimate": False},
+ {"point_estimate": "mean"},


It would be better to keep this case testing the no point estimate behaviour, which is achieved with None. "mean" is the default in rcParams, so it is actually tested many times.

Yes None makes more sense as rest three are tested many times. Previously, the tests are failing in pytest. I was getting the following ValueError:
ValueError: Point estimate should be 'mean', 'median', 'mode' or None, not False
Can you explain this behavior?

I'll make it None.

OriolAbril · 2020-01-13T20:59:39Z

arviz/tests/test_plots_bokeh.py

@@ -883,7 +883,7 @@ def test_plot_ppc_ax(models, kind):
 {"rope": {"mu": [{"rope": (-2, 2)}], "theta": [{"school": "Choate", "rope": (2, 4)}]}},
 {"point_estimate": "mode"},
 {"point_estimate": "median"},
- {"point_estimate": False},
+ {"point_estimate": "mean"},


same comment, None would be better.

percygautam · 2020-01-13T21:51:39Z

@OriolAbril I read the matplotlib files of plot_density and plot_posterior. I would like to add mode option to plot_density in this PR itself. We already got so many issues, I'd hate it to make another one if it can be solved here. Please guide me through it!

OriolAbril · 2020-01-13T22:24:48Z

Great! The currently currently has some duplication when calculating the point estimates. The skeleton of the code is basically some ifs clauses and inside the calculation of the point estimate using a vec/values variable (that is a 1d array).

It would be great to create a function in plot_utils similar to this:

def calculate_point_estimate(point_estimate, values, bw):
    # point_estimate validation and raise error here
    # if mean
        calculations
    # if median
        ...
    return point_value

The code inside the function would be more or less the same as the current code in posteriorplot. Then, when drawing the point estimate in plot_posterior and plot_density (both bokeh and matplotlib, so 4 files) the code would be simplified to calling this function (instead of the ifs) and then plotting (which I think won't need to be modified).

Also, both the check for good values plus error raising and getting the default from rcParams can also be moved to this new internal function.

percygautam · 2020-01-14T11:30:01Z

@OriolAbril I have done the changes you asked. Some tests were failing. Kindly review the work. I'll do black formatting as soon as tests passes

ahartikainen · 2020-01-14T11:42:44Z

Can you move _fast_kde functionality to plot_utils. There is an error in import order.

percygautam · 2020-01-14T12:01:36Z

@ahartikainen Okay I'll do that but a fourier transform method in plot utilities doesn't make sense. Isn't there any workaround to the problem.

ahartikainen · 2020-01-14T12:12:24Z

I think it can go there until we update our KDE code and move it to another location (outside plotting).

…ils.py

percygautam · 2020-01-14T13:09:59Z

@ahartikainen @OriolAbril , I have done the changes asked.

percygautam · 2020-01-14T14:24:13Z

@OriolAbril @ahartikainen The _fast_kde function is shoeing to be protected and can't be used outside directory plots.

OriolAbril · 2020-01-14T14:40:23Z

arviz/stats/stats.py

@@ -12,9 +12,8 @@
 from scipy.optimize import minimize
 import xarray as xr

+from ..plots.plot_utils import *


Make the import explicit from ..plots.plot_utils import _fast_kde, get_bins

@OriolAbril I already tried that, it is showing import error and the tests are not running

* will surely not import _fast_kde because * only imports public functions. Using explicit import should work though, just like it worked with from ..plots.kdeplot import _fast_kde which is also on another folder). Could there have been a typo? I currently cannot test it nor work on this locally, this is as much as I can do for now.

@OriolAbril I'll explicit import the function, so you can review!

* correct bfmi denominator * update test_diagnostics.py * fixed denominator and docs

percygautam · 2020-01-14T16:17:59Z

@OriolAbril @ahartikainen There was some the dependency issue which was causing the import error. All set now!

percygautam · 2020-01-14T16:58:02Z

@OriolAbril @ahartikainen There were linting issues. Should be okay now.

percygautam · 2020-01-14T18:12:08Z

@OriolAbril I am getting error with pylint No name 'gaussian' in module 'scipy.signal'.
I guess this is problem with pylint with gausian not registered.

OriolAbril

I have tinkered a little with the code.

Regarding the gaussian issue, it clearly should not happen, as scipy.signal.gaussian is still valid, there have been some movements towards its deprecation though, so now looks like a good moment to make the move and follow the warning in the docstring:

.. warning:: scipy.signal.gaussian is deprecated,
                 use scipy.signal.windows.gaussian instead.

Regarding the issue with _fast_kde import, I have commented a possible solution. It seemed to work locally.

OriolAbril · 2020-01-14T22:21:44Z

arviz/plots/backends/bokeh/densityplot.py

@@ -185,11 +189,8 @@ def _d_helper(
 ax.diamond(xmin, 0, line_color="black", fill_color=color, size=markersize)
 ax.diamond(xmax, 0, line_color="black", fill_color=color, size=markersize)

+ est = calculate_point_estimate(point_estimate, vec, bw)


I would move that inside the if, there is no need to calculate the point value if it will not be plotted

Yeah, makes sense. Will do.

OriolAbril · 2020-01-14T22:22:29Z

arviz/plots/backends/bokeh/posteriorplot.py

@@ -187,21 +187,9 @@ def display_rope(max_data):
 ax.text(x=vals, y=[max_data * 0.2, max_data * 0.2], text=list(map(str, vals)), **text_props)

 def display_point_estimate(max_data):
+ point_value = calculate_point_estimate(point_estimate, values, bw)


same comment, here it means after the if instead of inside though

OriolAbril · 2020-01-14T22:23:11Z

arviz/plots/backends/matplotlib/densityplot.py

@@ -155,11 +160,8 @@ def _d_helper(
 ax.plot(xmin, 0, hpd_markers, color=color, markeredgecolor="k", markersize=markersize)
 ax.plot(xmax, 0, hpd_markers, color=color, markeredgecolor="k", markersize=markersize)

+ est = calculate_point_estimate(point_estimate, vec, bw)


inside the if

OriolAbril · 2020-01-14T22:23:23Z

arviz/plots/backends/matplotlib/posteriorplot.py

@@ -179,21 +179,9 @@ def display_rope():
 ax.text(vals[1], plot_height * 0.2, vals[1], weight="semibold", **text_props)

 def display_point_estimate():
+ point_value = calculate_point_estimate(point_estimate, values, bw)


after the if

OriolAbril · 2020-01-14T22:26:37Z

arviz/stats/stats.py

 from ..data import convert_to_inference_data, convert_to_dataset, InferenceData, CoordSpec, DimSpec
-from ..plots.kdeplot import _fast_kde
-from ..plots.plot_utils import get_bins
 from .diagnostics import _multichain_statistics, _mc_error, ess, _circular_standard_deviation
 from .stats_utils import (
 make_ufunc as _make_ufunc,


below on line 27, histogram is imported from stats_utils in a weird way, it can be moved to this import.

I think this triggers some kind of circular import and is the root of the import problems

@OriolAbril I noticed this problem earlier too and has removed the import problem with b2f600d commit.

Yes, but the code should use the histogram function, not rewrite it here from scratch.

percygautam · 2020-01-15T12:22:11Z

@OriolAbril Where do we need to add warning in the docstring? I have added it in the function _fast_kde as it is using gaussian function. The other changes you asked are done.

OriolAbril · 2020-01-15T12:55:18Z

The warning is in the docstring of scipy.signal.gaussian, I was thinking we could follow its advise and start using scipy.signal.windows.gaussian instead.

Also, now tests seem to pass and ArviZ can be imported, but we should move the from ..stats.stats_utils import histogram to the imports right above:

from .stats_utils import (
    make_ufunc as _make_ufunc,
    wrap_xarray_ufunc as _wrap_xarray_ufunc,
    logsumexp as _logsumexp,
    ELPDData,
    stats_variance_2d as svar,
    histogram  # here is its proper place
)

percygautam · 2020-01-15T13:22:32Z

@OriolAbril Thanks for the clarification. I didn't understand the problem earlier. I did the changes asked.

ahartikainen · 2020-01-16T07:31:26Z

arviz/plots/__init__.py

@@ -9,7 +9,7 @@
 from .forestplot import plot_forest
 from .hpdplot import plot_hpd
 from .jointplot import plot_joint
-from .kdeplot import plot_kde, _fast_kde, _fast_kde_2d
+from .kdeplot import plot_kde, _fast_kde_2d


You can also move _fast_kde_2d next to _fast_kde, sorry for not saying it explicitly before

percygautam · 2020-01-16T11:54:50Z

@ahartikainen Should I do these changes in this PR itself or create another one after this is merged. Transferring _fast_kde_2d to plot_utils.py will again cause dependency issues as they did earlier with _fast_kde.

OriolAbril · 2020-01-16T21:59:25Z

After modifying the import of histogram from stats_utils should be safe to move fast_kde_2d, we can do it here.

percygautam · 2020-01-17T12:39:39Z

@OriolAbril I am moving all derived private functions _cov_1d, _cov and _dot in _fast_kde_2d to plot_utils.py, as it would make sense. Is it okay?

OriolAbril · 2020-01-17T15:28:47Z

LGTM

ahartikainen

LGTM

percygautam added 3 commits January 14, 2020 01:59

integrated the point_estimate param with rcparam

eb0af7a

added black formatting

1c9f1c9

added black formatting

3f336fe

OriolAbril reviewed Jan 13, 2020

View reviewed changes

created new function calculate_point_estimate

25083ba

changed the location of _fast_kde function from kdeplot.py to plot_ut…

c4029fd

…ils.py

OriolAbril reviewed Jan 14, 2020

View reviewed changes

nitishp25 and others added 3 commits January 14, 2020 21:13

Correct bfmi denominator (arviz-devs#991)

18a1dc4

* correct bfmi denominator * update test_diagnostics.py * fixed denominator and docs

changes

f36ae5e

dependency issue solved

b2f600d

linting changes

1d6f174

linting changes

009145d

OriolAbril reviewed Jan 14, 2020

View reviewed changes

changes

ec3374b

percygautam added 2 commits January 15, 2020 18:52

final changes

49ab1e5

linting correction

b344445

OriolAbril approved these changes Jan 15, 2020

View reviewed changes

ahartikainen reviewed Jan 16, 2020

View reviewed changes

moved _fast_kde_2d function to plot_utils.py

3a8c621

percygautam requested a review from OriolAbril January 17, 2020 13:04

minor linting changes

dc138d2

OriolAbril approved these changes Jan 17, 2020

View reviewed changes

percygautam changed the title ~~[WIP] Solve issue #992 : Integrate point_estimate with rcParam~~ Solve issue #992 : Integrate point_estimate with rcParam Jan 17, 2020

ahartikainen approved these changes Jan 17, 2020

View reviewed changes

ahartikainen merged commit 4cfe800 into arviz-devs:master Jan 17, 2020

percygautam deleted the point_estimate branch January 20, 2020 19:00

Solve issue #992 : Integrate point_estimate with rcParam #994

Solve issue #992 : Integrate point_estimate with rcParam #994

Conversation

percygautam commented Jan 13, 2020 • 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

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

percygautam commented Jan 13, 2020 • edited Loading

OriolAbril commented Jan 13, 2020

percygautam commented Jan 14, 2020

ahartikainen commented Jan 14, 2020

percygautam commented Jan 14, 2020

ahartikainen commented Jan 14, 2020

percygautam commented Jan 14, 2020

percygautam commented Jan 14, 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

percygautam commented Jan 14, 2020

percygautam commented Jan 14, 2020

percygautam commented Jan 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

percygautam commented Jan 15, 2020

OriolAbril commented Jan 15, 2020

percygautam commented Jan 15, 2020

Choose a reason for hiding this comment

percygautam commented Jan 16, 2020

OriolAbril commented Jan 16, 2020

percygautam commented Jan 17, 2020

OriolAbril commented Jan 17, 2020

ahartikainen left a comment

Choose a reason for hiding this comment

percygautam commented Jan 13, 2020 •

edited

Loading

percygautam commented Jan 13, 2020 •

edited

Loading