Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] fix hdi false positive warning #1241

Merged
merged 11 commits into from
Jun 22, 2020
Merged

[WIP] fix hdi false positive warning #1241

merged 11 commits into from
Jun 22, 2020

Conversation

OriolAbril
Copy link
Member

@OriolAbril OriolAbril commented Jun 13, 2020

Description

hdi used to support 2d data at most, so plot_hdi converted the data to 2d. Thanks to #1117 this is not necessary anymore and triggers this warning.

I have also done a couple of updates to summary also as a followup of #1117. I want to note that now az.hdi and xr.Dataset.quantile return an object with the same format and we can therefore use the same pattern as hdi now uses to report also equal tail intervals.

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

@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #1241 into master will increase coverage by 14.87%.
The diff coverage is 89.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1241       +/-   ##
===========================================
+ Coverage   78.27%   93.14%   +14.87%     
===========================================
  Files          98       98               
  Lines        9726     9721        -5     
===========================================
+ Hits         7613     9055     +1442     
+ Misses       2113      666     -1447     
Impacted Files Coverage Δ
arviz/plots/plot_utils.py 94.57% <ø> (ø)
arviz/plots/hdiplot.py 89.23% <83.87%> (+5.30%) ⬆️
arviz/plots/backends/bokeh/hdiplot.py 100.00% <100.00%> (ø)
arviz/plots/backends/matplotlib/hdiplot.py 92.30% <100.00%> (+7.69%) ⬆️
arviz/stats/stats.py 96.24% <100.00%> (+0.03%) ⬆️
arviz/plots/pairplot.py 92.77% <0.00%> (ø)
arviz/utils.py 89.56% <0.00%> (+0.35%) ⬆️
arviz/data/inference_data.py 80.11% <0.00%> (+1.15%) ⬆️
arviz/data/base.py 100.00% <0.00%> (+10.97%) ⬆️
arviz/data/io_cmdstan.py 94.84% <0.00%> (+51.07%) ⬆️
... and 8 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 32be6fc...c1af041. Read the comment docs.

@OriolAbril OriolAbril changed the title fix hdi false positive warning [WIP] fix hdi false positive warning Jun 13, 2020
@AlexAndorra
Copy link
Contributor

AlexAndorra commented Jun 13, 2020 via email

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! Just added some comments 😉

arviz/plots/hdiplot.py Outdated Show resolved Hide resolved
arviz/plots/hdiplot.py Show resolved Hide resolved
arviz/plots/hdiplot.py Show resolved Hide resolved
Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good now, thanks @OriolAbril 👌

arviz/plots/hdiplot.py Outdated Show resolved Hide resolved
Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com>
@OriolAbril
Copy link
Member Author

Ready to merge, if nobody complains I'll merge tomorrow

@OriolAbril
Copy link
Member Author

I guess there are a couple points that could be a bit more problematic:

  • I am setting the coordinate values in hdi dim to ["lower", "higher"] which does not include the hdi_prob info (this diverges from summary df for instance, I find hdi_3% a little confusing). I have included hdi_prob as an attribute in case it were needed.
  • passing 2d data to hdi will work and be backwards compatible but I have added a FutureWarning because it does not comply with ArviZ chain, draw, *shape convention. In a future release we should remove FutureWarning and enforce the convention to make hdi behave coherently with all other ArviZ functions

Extra easter egg: now az.hdi's behaviour is very similar to the point that using hdi_data allows to pass the result of Dataset.quantile to az.plot_hdi (a transpose may be needed to make sure the quantile/hdi dimension is the last one)

@AlexAndorra
Copy link
Contributor

That looks good to me 👌 Can be included in 0.8.4

@ahartikainen ahartikainen merged commit c1190ca into master Jun 22, 2020
@OriolAbril OriolAbril deleted the hdi_warning branch June 22, 2020 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants