Revamping HPD by percygautam · Pull Request #1117 · arviz-devs/arviz

percygautam · 2020-03-14T12:10:22Z

Description

fixes #855 Make hpd work with multidimensional arrays.

Checklist

Follows official PR format
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

OriolAbril

Multimodal case is very tricky, I don't think it is possible to know the shape beforehand. i think the best way to tackle it would be to create the ufunc by hand and then use xr.apply_ufunc directly. This would avoid using make_ufunc that requires creating the output array beforehand.

However, I would first work on unimodal case and once unimodal is up and running, move onto multimodal.

percygautam · 2020-03-19T19:51:45Z

I have made the changes. It is working fine for ndarray and datasets. Currently, I am first converting ndarray to dataset then using wrap_xarray_ufunc.
I tried using wrap_xarray_ufuncdirectly on ndarray but the hpd is calculated is always calculated on the last dimension. Converting it to ndarray gives me control over which dimension to calculate hpd for.
Any workaround for this?

OriolAbril

I tried using wrap_xarray_ufuncdirectly on ndarray but the hpd is calculated is always calculated on the last dimension. Converting it to ndarray gives me control over which dimension to calculate hpd for.
Any workaround for this?

I think this is the best workaround, it gives complete control and it is quite explicit. The other option would be to manually reorder ndarray dimensions, but I would not recommend it as it is much less readable.

I think that unimodal case is nearly finished, only things missing are tests and some nits. Regarding multimodal case, I was thinking of a possible workaround, let's see what everyone thinks about this. The idea would be to have _hpd_multimodal return a result with shape (2, 10) (the 10 is completely arbitrary and could be modified, even be an argument). Of these 10 pairs of values, the first would be hpd intervals and the last ones will probably not be needed, and would be then set to nan (in _hpd_multimodal). Eventually hpd in the case of multimodal would drop nan values and return the hpd dataset with the proper shape (also, different variables may have different number of modes and this approach should solve this too).

percygautam · 2020-03-22T21:10:03Z

Do we start the work of multimodal here or wait for everyone's opinion? Maybe create a new issue for multimodal discussion.

codecov · 2020-03-22T21:38:31Z

Codecov Report

Merging #1117 into master will decrease coverage by 0.00%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #1117      +/-   ##
==========================================
- Coverage   92.68%   92.67%   -0.01%     
==========================================
  Files          93       93              
  Lines        9073     9097      +24     
==========================================
+ Hits         8409     8431      +22     
- Misses        664      666       +2

Impacted Files	Coverage Δ
arviz/stats/stats.py	`96.32% <89.47%> (-0.21%)`	⬇️

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 9909aab...b746b9b. Read the comment docs.

OriolAbril · 2020-03-23T23:29:31Z

I would work on multimodal in this same PR, otherwise, behaviour of ArviZ development version between merging this and the other will be quite confusing.

percygautam · 2020-03-24T12:01:28Z

Regarding multimodal case, I was thinking of a possible workaround, let's see what everyone thinks about this. The idea would be to have _hpd_multimodal return a result with shape (2, 10) (the 10 is completely arbitrary and could be modified, even be an argument). Of these 10 pairs of values, the first would be hpd intervals and the last ones will probably not be needed, and would be then set to nan (in _hpd_multimodal). Eventually hpd in the case of multimodal would drop nan values and return the hpd dataset with the proper shape (also, different variables may have different number of modes and this approach should solve this too).

Should I start implementing this or wait?

OriolAbril · 2020-03-24T15:41:25Z

Let's go ahead with multimodal 💪

percygautam · 2020-03-25T19:03:42Z

I have tried to implement the multimodal case for a single input. Currently, I am filling only the first dimension with NaNs.
How do we decide the size (10) of the first dimension in this case?

OriolAbril

I tried to comment everything on the related piece of code, but most of the comments are related one to the other and even apply to several places, read everything first and ask if there is anything unclear.

OriolAbril

Looks great!

I have one question about ndarray input with ndims > 2. This is currently not supported right?

I am not sure if we should extend the 2d behaviour (as the code does now) or assume ArviZ dimensions of (chain, draw, *shape). I am inclined towards the second but we should probably weigh the pros and cons and reach some kind of consensus.

percygautam · 2020-03-27T17:41:38Z

I think ndarray input with ndims > 2 is not supported.
P.S. can you check the linting error. I already popped out_shape, yet it's showing unexpected keyword.

OriolAbril · 2020-03-27T20:28:03Z

You can add a comment to ignore the pylint warning, it probably misses the pop

OriolAbril

I think these comments will be the last nits

OriolAbril · 2020-03-29T17:39:49Z

+    func = _hpd_multimodal if multimodal else _hpd

-        density *= dx
+    if isinstance(ary, np.ndarray):


This should be only if the array is 1d or 2d:

isarray = isinstance(ary, np.ndarray) if isarray and ary.ndim <= 2:

If the array has 3 or more dimensions, it should assume ArviZ dim order: (chain, draw, *shape). hpd should still return a numpy array though:

... hpd_data = _wrap_xarray_ufunc(func, ary, func_kwargs=func_kwargs, **kwargs) hpd_data = hpd_data.dropna("mode", how="all") if multimodal else hpd_data return hpd_data.x.values if isarray else hpd_data

OriolAbril · 2020-03-29T17:40:56Z

+def test_hpd_multidimension():
+    normal_sample = np.random.randn(12000, 10, 3)
+    result = hpd(normal_sample)
+    assert result.shape == (10, 3, 2,)


This line will have to be updated to check that the result shape is the desired (3, 2)

Earlier, we were calculating hpd over one dimension only, for ndarrays. So, for backward compatibility I have set default to be calculated only over 'chain' for ndarrays. So, the result still would be (10, 3, 2,).

The issue is that calculating hpd only over chain is a very bad default, we'll keep the behaviour (for now) in 2d array case to keep backwards compatibility, but 3d arrays are not supported, so we do not have the backwards compatibility constraint.

Okay, I have done the changes.

mjhajharia · 2021-03-28T15:52:11Z

i ran some cell with pm.stats.hpd in an example notebook and ig its removed or something cause i get the error - no attribute 'hpd'. I tried az.hpd too, same thing. im probably missing something, is the function renamed or stuff.

ahartikainen · 2021-03-28T17:20:26Z

Did you try az.hdi? Please open a new issue if that doesn't work.

mjhajharia · 2021-03-29T10:19:19Z

@ahartikainen thankyou that worked!

percygautam added 5 commits March 12, 2020 01:34

revamped hpd function

d29cefb

corrected hpd errors

2bb5646

add docstring

7de2518

linting change

e4d8318

minor nits

ae2e59d

OriolAbril reviewed Mar 14, 2020

View reviewed changes

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/stats/stats.py Outdated

allow input to be ndarray/dataset

f2e67b4

OriolAbril reviewed Mar 20, 2020

View reviewed changes

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/tests/base_tests/test_stats.py

Comment thread arviz/stats/stats.py

add tests for unimodal case

6dd3ee8

percygautam changed the title ~~Make hpd work with multidimensional arrays~~ Revamping HPD Mar 20, 2020

OriolAbril reviewed Mar 20, 2020

View reviewed changes

Comment thread arviz/stats/stats.py Outdated

Comment thread arviz/tests/base_tests/test_stats.py Outdated

Comment thread arviz/tests/base_tests/test_stats.py

percygautam added 4 commits March 21, 2020 01:36

test group and other minor nits for unimodal

1d75d93

Merge branch 'master' into hpd

9f7ad1f

corrected failed tests

a24323e

added sel argument and its tests

ff176f4

OriolAbril reviewed Mar 22, 2020

View reviewed changes

Comment thread arviz/stats/stats.py

minor nits

82f15a4

pydocstyle change

0ff7d18

OriolAbril reviewed Mar 23, 2020

View reviewed changes

Comment thread arviz/tests/base_tests/test_stats.py Outdated

Comment thread arviz/tests/base_tests/test_stats.py Outdated

minor changes

deaf5e9

hpd multimodal

c069dd5

OriolAbril reviewed Mar 26, 2020

View reviewed changes

changes to hpd multimodal

000866c

minor nits

e9d6ace

OriolAbril reviewed Mar 26, 2020

View reviewed changes

Comment thread arviz/stats/stats.py

Comment thread arviz/stats/stats.py Outdated

nits

2aa68fe

percygautam added 3 commits March 28, 2020 02:26

final nits

cdea066

Merge branch 'master' into hpd

9e6ac55

add to changelog

6cfee48

OriolAbril reviewed Mar 29, 2020

View reviewed changes

percygautam added 3 commits March 30, 2020 23:12

changes

19beaf2

final changes

758e9b5

Merge branch 'master' into hpd

b746b9b

OriolAbril approved these changes Mar 31, 2020

View reviewed changes

aloctavodia merged commit aca28da into arviz-devs:master Apr 2, 2020

percygautam deleted the hpd branch June 1, 2020 09:51

OriolAbril mentioned this pull request Jun 13, 2020

[WIP] fix hdi false positive warning #1241

Merged

5 tasks

mjhajharia mentioned this pull request Mar 28, 2021

rugby analytics pymc-devs/pymc-examples#49

Closed

Uh oh!

Conversation

percygautam commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

percygautam commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

percygautam commented Mar 22, 2020

Uh oh!

codecov Bot commented Mar 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

OriolAbril commented Mar 23, 2020

Uh oh!

percygautam commented Mar 24, 2020

Uh oh!

OriolAbril commented Mar 24, 2020

Uh oh!

percygautam commented Mar 25, 2020

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

percygautam commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OriolAbril commented Mar 27, 2020

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OriolAbril Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

OriolAbril Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

percygautam Mar 30, 2020

percygautam commented Mar 14, 2020 •

edited

Loading

percygautam commented Mar 19, 2020 •

edited

Loading

codecov Bot commented Mar 22, 2020 •

edited

Loading

percygautam commented Mar 27, 2020 •

edited

Loading