Refactor plot_ecdf arguments

sethaxen commented 4 months ago

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_bands now may take string arguments as well as boolean
ci_prob specifies band probability. stats.ci_prob is a new rcParam.
eval_points allows the user to specify the evaluation points
rvs, random_state can be provided for simulation confidence bands

Deprecated arguments:

values2 is now deprecated, in favor of users passing an empirical CDF

Deprecated keywords:

pointwise is now confidence_bands="pointwise"
fpr is replaced with 1-ci_prob for consistency with other plotting functions.
pit is 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.
npoints

Deprecated rcparams:

stats.hdi_prob has been deprecated and replaced with stats.ci_prob

Additional changes:

If eval_points not provided, there's a warning that in the future eval_points will 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[bot] commented 4 months ago

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

```diff @@ 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.

sethaxen commented 4 months ago

@OriolAbril when you get a chance, can you take a look at this?

review-notebook-app[bot] commented 3 months ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

sethaxen commented 3 months ago

@OriolAbril I've implemented all suggestions and updated the above description and changelog. This should be ready for final review.

sethaxen commented 3 months ago

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.

OriolAbril commented 3 months ago

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?

OriolAbril commented 3 weeks ago

@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

OriolAbril commented 3 weeks ago

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

arviz-devs / arviz