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

Keep replacing hpd with hdi #1190

Merged
merged 3 commits into from
May 18, 2020
Merged

Keep replacing hpd with hdi #1190

merged 3 commits into from
May 18, 2020

Conversation

aloctavodia
Copy link
Contributor

@aloctavodia aloctavodia commented May 18, 2020

Fix #1186

  • Follows official PR format
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@aloctavodia aloctavodia changed the title replace hpd with hdi Keep replacing hpd with hdi May 18, 2020
@canyon289
Copy link
Member

canyon289 commented May 18, 2020

You are too kind @aloctavodia. An hpd function needs to exist or pymc3 import breaks tests. Likely need one anyway with a deprecation warning. At some point when hpd becomes actually deprecated we need to coordinate two merges across pymc3 and arviz to remove from both.

hpd_fill_kwargs.setdefault("label", "Uniform HPD")
hpd_kwargs["fill_kwargs"] = hpd_fill_kwargs
hpd_kwargs["credible_interval"] = credible_interval
if use_hdi:
Copy link
Member

Choose a reason for hiding this comment

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

for loo pit I left it hpd because this particular case it is hpd. I don't think loo pit makes sense for anything bu the posterior. Not opposed to changing it but wanted to get your thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's true, but having hpd and hdi seems to me like a source of potential confusion for users or even ourselves in the future

@@ -184,7 +184,7 @@ def plot_posterior(

>>> az.plot_posterior(data, var_names=['mu'], kind='hist')

Change size of HPD interval
Change size of highest density interval
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching my misses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not remove any function (at least not on purpose, haha) I just marked plot_hpd as deprecated in favor of plot_hdi. And I replaced the strings hpd to hdi.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes you're right I see it now



def plot_hpd(*args, **kwargs):
warnings.warn("plot_hdi has been deprecated, please use plot_hdi", DeprecationWarning)
Copy link
Member

Choose a reason for hiding this comment

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

Pydocstyle complaining about this one method. Maybe add disable or deprecation comment to get a pass

@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #1190 into master will increase coverage by 0.02%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1190      +/-   ##
==========================================
+ Coverage   93.23%   93.26%   +0.02%     
==========================================
  Files          94       94              
  Lines        9376     9381       +5     
==========================================
+ Hits         8742     8749       +7     
+ Misses        634      632       -2     
Impacted Files Coverage Δ
arviz/plots/backends/matplotlib/densityplot.py 96.42% <ø> (ø)
arviz/plots/backends/matplotlib/forestplot.py 97.93% <ø> (+1.39%) ⬆️
arviz/plots/densityplot.py 91.93% <ø> (ø)
arviz/plots/posteriorplot.py 90.24% <ø> (ø)
arviz/plots/violinplot.py 88.57% <ø> (ø)
arviz/stats/stats.py 96.20% <ø> (ø)
arviz/plots/hdiplot.py 87.50% <80.00%> (ø)
arviz/plots/__init__.py 100.00% <100.00%> (ø)
arviz/plots/backends/bokeh/__init__.py 87.17% <100.00%> (ø)
arviz/plots/backends/bokeh/forestplot.py 92.88% <100.00%> (ø)
... 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 4db8674...f0c69ab. Read the comment docs.

@canyon289
Copy link
Member

LGTM merging

@canyon289 canyon289 merged commit a6ea9e5 into master May 18, 2020
@OriolAbril OriolAbril deleted the hdi branch May 24, 2020 14:21
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.

Change plot_hpd to plot_hdi (with a deprecation warning)
2 participants