Refactor plot_ecdf arguments
Description
This PR introduces new keyword arguments to plot_ecdf and deprecates a few existing ones, following suggestions in #2309.
New keywords and features:
confidence_bandsnow may take string arguments as well as booleanband_probspecifies band probability.plot.band_probis a new rcParam.eval_pointsallows the user to specify the evaluation pointsrvs,random_statecan be provided for simulation confidence bands
Deprecated keywords:
pointwiseis nowconfidence_bands="pointwise"fpris replaced with1-band_probfor consistency with other plotting functions.pitis deprecated. We only need this for LOO-PIT, users who need it for something else will probably know how to make the plot, and if we really want to include it, it should be its own plotting function. There's now a documented example of how to plot PIT.
Additional changes:
- If
eval_pointsnot provided, there's a warning that in the futureeval_pointswill be the unique values of the sample. This would be breaking and is saved for a future release.
None of the changes are breaking.
Checklist
- [x] Follows official PR format
- [x] Includes a sample plot to visually illustrate the changes (only for plot-related functions)
- [x] New features are properly documented (with an example if appropriate)?
- [x] Includes new or updated tests to cover the new feature
- [x] Code style correct (follows pylint and black guidelines)
- [x] Changes are listed in changelog
📚 Documentation preview 📚: https://arviz--2316.org.readthedocs.build/en/2316/
Codecov Report
Attention: Patch coverage is 90.24390% with 8 lines in your changes missing coverage. Please review.
Project coverage is 87.01%. Comparing base (
3a454f7) to head (f414ab0).
| Files | Patch % | Lines |
|---|---|---|
| arviz/rcparams.py | 71.42% | 4 Missing :warning: |
| arviz/plots/ecdfplot.py | 94.00% | 3 Missing :warning: |
| arviz/plots/bpvplot.py | 0.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #2316 +/- ##
==========================================
+ Coverage 86.97% 87.01% +0.04%
==========================================
Files 123 123
Lines 12733 12771 +38
==========================================
+ Hits 11074 11113 +39
+ Misses 1659 1658 -1
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@OriolAbril when you get a chance, can you take a look at this?
Check out this pull request on ![]()
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
@OriolAbril I've implemented all suggestions and updated the above description and changelog. This should be ready for final review.
After seeing the warnings and trying it out I am a bit on the fence on the behaviour of
eval_points. It is basically a required argument right now, otherwise you get a FutureWarning.It would be nice to continue allowing plot_ecdf(samples) to work without warnings.
The reason it raises a FutureWarning is because in the future plot_ecdf(samples) will do something entirely different than it does now. The alternative is that we currently don't raise a warning and instead change the behavior in the future without warning. Personally, I think the way we do it here could be less jarring.
Maybe we could create a specific warning class for this? Something like BehaviourChangeWarning, I think it might signal better what is happening and also make it easier to silence (I am quite sure many users don't really care about the default as long as it works) and plot_ecdf(samples) will continue to work.
We could also silence it in the examples of the docstring. Now all examples use eval_points, but to illustrate how to generate confidence bands or how to make it a difference plot it doesn't matter which is the default behaviour of eval_points (and tests don't use it). So we could use the first example to describe the behaviour change, show how to maintain old behaviour and then silence the warning so following examples focus on what they want to illustrate without worrying about the warning.
What do you think?
@sethaxen I have tried out the special warning and added the filter to the docs. Now we should make sure all examples in the docstring don't trigger any warning, I have to go now, so leaving this here so when I come back later I can check the docs preview instead of locally building it myself at some point
Should be ready to merge now. There is one example in the docstring that triggers a warning, the one for
Plot an ECDF plot with confidence bands for comparing a given sample to a given distribution. We manually specify evaluation points independent of the values so that the confidence bands are correctly calibrated.
because rvs is not provided, I think this is only temporal though, is that right? Once the new default is available there won't be a warning when using that same code snippet