Refactor plot_ecdf arguments #2316

sethaxen · 2024-02-23T11:27:11Z

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

Follows official PR format
Includes a sample plot to visually illustrate the changes (only for plot-related functions)
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

📚 Documentation preview 📚: https://arviz--2316.org.readthedocs.build/en/2316/

arviz/plots/ecdfplot.py

sethaxen · 2024-02-23T14:13:46Z

arviz/plots/ecdfplot.py

+          band.
+        For simultaneous confidence bands to be correctly calibrated, provide `eval_points` that
+        are not dependent on the `values`.
+    band_prob : float, default 0.95


I'd argue that this should default to 0.94 for consistency with hdi_prob, but that would be breaking (since fpr was 0.05).

This will ultimately depend on the hdi_prob, band_prob, ci_prob discussion, but in my opinion changing the value of hdi_prob (or any other rcParam defined probability) is not a breaking change. It is documented in multiple places that these are completely arbitrary values and that might also change.

The only guarantee should be that is someone was using fpr=0.05 it still works for a while but changing the probability of the band they get when not providing fpr is ok.

sethaxen · 2024-02-23T14:15:08Z

arviz/rcparams.py

@@ -262,6 +262,7 @@ def validate_iterable(value):
        "mean",
        _make_validate_choice({"mean", "median", "mode"}, allow_none=True),
    ),
+    "plot.band_prob": (0.95, _validate_probability),


Potentially this should be stats.band_prob, especially if we add the ECDF confidence band computation functions to the API.

sethaxen · 2024-02-23T14:15:33Z

arviz/stats/ecdf_utils.py

@@ -90,7 +90,7 @@ def ecdf_confidence_band(
        A function that takes an integer `ndraws` and optionally the object passed to
        `random_state` and returns an array of `ndraws` samples from the same distribution
        as the original dataset. Required if `method` is "simulated" and variable is discrete.
-    num_trials : int, default 1000
+    num_trials : int, default 500


Just changed for consistency with original behavior.

sethaxen · 2024-02-23T14:17:58Z

arviz/plots/ecdfplot.py

@@ -46,26 +52,41 @@ def plot_ecdf(
    values : array-like
        Values to plot from an unknown continuous or discrete distribution.
    values2 : array-like, optional
-        Values to compare to the original sample.
+        deprecated: values to compare to the original sample. Instead use
+        `cdf=scipy.stats.ecdf(values2).cdf.evaluate`.


I hope in a future PR to add an ECDF-(difference)plot option to plot_ranks and then recommend that here.

sethaxen · 2024-02-23T14:18:23Z

arviz/plots/ecdfplot.py

+    confidence_bands : str or bool, optional
+        - False: No confidence bands are plotted.
+        - "pointwise": Compute the pointwise (i.e. marginal) confidence band.
+        - True or "simulated": Use Monte Carlo simulation to estimate a simultaneous confidence


In the next PR I will add deterministic bands, which will become the default here.

I would then put "True" and "simulated" on different lines. True saying bands are computed with the default algorithm (subject to change), and simulated can keep the current description.

codecov · 2024-02-23T15:42:52Z

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).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
arviz/rcparams.py	71.42%	4 Missing ⚠️
arviz/plots/ecdfplot.py	94.00%	3 Missing ⚠️
arviz/plots/bpvplot.py	0.00%	1 Missing ⚠️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sethaxen · 2024-03-03T07:30:05Z

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

OriolAbril

Sorry for the slowness, still a bit fuzzy on some of the warnings, I'll try to use the PR tomorrow to make sure I see what happens given the different combinations.

OriolAbril · 2024-03-07T14:28:49Z

CHANGELOG.md

+-   Added arguments `band_prob`, `eval_points`, `rvs`, and `random_state` to `plot_ecdf` ([2316](https://github.com/arviz-devs/arviz/pull/2316))
+-   Added rcParam `plot.band_prob` ([2316](https://github.com/arviz-devs/arviz/pull/2316))


I was going to comment "not sure why the band_prob parameter can't be ci_prob" then realized the rcparam change was at arviz-base only, not for current arviz. Given the plans to change that eventually, do you think it would be better to deprecate and do the change here already?

We could do that change here, sure. I'd avoid changing it elsewhere until a future PR.

RE the change in arviz-base, why the switch to default CI of eti instead of hdi?

RE the change in arviz-base, why the switch to default CI of eti instead of hdi?

Mostly because I hadn't ported hdi yet, less so to switch things up

Okay, so to clarify, is the idea that we replace stats.hdi_prob and the new plot.band_prob with a new stats,ci_prob? If so, is there a procedure for deprecating rcparams so that users can get an informative warning if they try to set hdi_prob?

I don't think we have ever changed an rcParam key, only a couple values. We should definitely add a deprecation warning and keep both around for a bit.

A proposal could be to have ci_prob behave like current hdi_prob and hdi_prob now instead takes an extra value: ci_prob (new default). If the value for hdi_prob is different than that then raise a FutureWarning. I think we can achieve that with a custom validation function relatively easily.

Here's what I did, roughly patterned after matplotlibs's own handing of deprecated rcparams: f6cff76

Effectively, the hdi_prob rcparam is now an alias for ci_prob, which has a default. Anytime someone sets or gets hdi_prob, a deprecation warning is raised. The implementation is flexible enough to support in the future more complicated deprecations. At the moment this raises plenty of deprecation warnings in our tests/docs builds, since hdi_prob is regularly accessed. But before changing any of those things, It'd be nice to get feedback on this approach.

This is great, much better than my proposal. The only comment is the emitted warnings should be FutureWarning (user facing) instead of DeprecationWarning (downstream dev facing). I don't really know how or why but this is Python convention and by default users don't even see DeprecationWarning unless explicitly activated. Ref https://docs.python.org/3/library/warnings.html#warning-categories

Ah, good to know, thanks! Done.

arviz/plots/ecdfplot.py

OriolAbril · 2024-03-07T14:33:09Z

arviz/plots/ecdfplot.py

+    confidence_bands : str or bool, optional
+        - False: No confidence bands are plotted.
+        - "pointwise": Compute the pointwise (i.e. marginal) confidence band.
+        - True or "simulated": Use Monte Carlo simulation to estimate a simultaneous confidence


I would then put "True" and "simulated" on different lines. True saying bands are computed with the default algorithm (subject to change), and simulated can keep the current description.

arviz/plots/ecdfplot.py

OriolAbril · 2024-03-07T14:40:21Z

arviz/plots/ecdfplot.py

+          band.
+        For simultaneous confidence bands to be correctly calibrated, provide `eval_points` that
+        are not dependent on the `values`.
+    band_prob : float, default 0.95


This will ultimately depend on the hdi_prob, band_prob, ci_prob discussion, but in my opinion changing the value of hdi_prob (or any other rcParam defined probability) is not a breaking change. It is documented in multiple places that these are completely arbitrary values and that might also change.

The only guarantee should be that is someone was using fpr=0.05 it still works for a while but changing the probability of the band they get when not providing fpr is ok.

arviz/plots/ecdfplot.py

review-notebook-app · 2024-04-04T09:15:15Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

sethaxen · 2024-04-04T13:07:40Z

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

OriolAbril

Missed some DeprecationWarnings that are user-facing. I'll batch commit the suggestions and merge, all the changes are extremely minor

CHANGELOG.md

arviz/plots/ecdfplot.py

arviz/tests/base_tests/test_plots_matplotlib.py

arviz/plots/ecdfplot.py

OriolAbril

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.

sethaxen · 2024-04-04T18:13:43Z

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 · 2024-04-04T18:38:09Z

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?

changing user-facing DeprecationWarnings to FutureWarnings

OriolAbril · 2024-06-10T16:21:24Z

@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 · 2024-06-11T13:37:15Z

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

sethaxen marked this pull request as ready for review February 23, 2024 14:11

sethaxen commented Feb 23, 2024

View reviewed changes

sethaxen requested a review from OriolAbril February 23, 2024 14:52

OriolAbril reviewed Mar 7, 2024

View reviewed changes

sethaxen requested a review from OriolAbril April 4, 2024 13:07

OriolAbril approved these changes Apr 4, 2024

View reviewed changes

OriolAbril reviewed Apr 4, 2024

View reviewed changes

arviz/plots/ecdfplot.py Outdated Show resolved Hide resolved

OriolAbril reviewed Apr 4, 2024

View reviewed changes

sethaxen added 14 commits June 10, 2024 17:35

Add rcparam for band probability

d93f1fa

Unify plot_ecdf keywords and deprecate old ones

f2ccb30

Change default num_trials to match plot_ecdf

5369471

Support keywords rvs and random_state

5e63b72

Deprecate values2

3026bd9

Allow eval_points to be specified

1ca6892

Restore original name confidence_bands

fddd415

Correctly specify ecdf usage

a99d6a2

Include recommended specification of eval_points

4f2dab9

Update examples in docstrings

495e29f

Deprecate pit keyword

7679805

Ravel values2

6602775

Change band_prob default to 0.95

43fce45

Run black

2ae21d7

sethaxen and others added 23 commits June 10, 2024 17:42

Remove duplicate warnings import

9ebfcd0

Fix linting errors

44d971b

Add missing newline

f2d9fa2

Mark valus2 as deprecated

4e4be74

Fix bulleted list of confidence_bands options

13e0863

Clean up acceptable types for random_state

194d3b9

Use deprecated directive for other keywords

750e2b5

Make deprecation note more specific

13b09fe

Deprecate npoints

7659fb9

Move deprecated keywords to end of keywords list

c38e45e

Update CHANGELOG.md

b9190f1

Add missing newline

c6131ff

Rename band_prob to ci_prob

effd669

Change ci_prob default to 0.94

3a75252

Deprecate hdi_prob and replace with stats.ci_prob

8b869bc

Support rvs with no keywords

bd472e7

Unify docstrings

6fba0a8

Change deprecation warning to future warning

257e0c0

Test deprecated rcparams

766971f

Use stats.ci_prob in plots

ef7a734

Use stats.ci_prob in docs

6985873

Don't access protected class variable

1669d66

Apply suggestions from code review

7f25a2c

changing user-facing DeprecationWarnings to FutureWarnings

OriolAbril force-pushed the ecdf_dep_kwargs branch from 635a35a to 7f25a2c Compare June 10, 2024 15:45

use BehaviourChangeWarning

6557824

black and docstring updates

f414ab0

OriolAbril merged commit 3453abd into main Jun 11, 2024
12 checks passed

OriolAbril deleted the ecdf_dep_kwargs branch June 11, 2024 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor plot_ecdf arguments #2316

Refactor plot_ecdf arguments #2316

sethaxen commented Feb 23, 2024 •

edited

Loading

sethaxen Feb 23, 2024

OriolAbril Mar 7, 2024

sethaxen Feb 23, 2024

sethaxen Feb 23, 2024

sethaxen Feb 23, 2024

sethaxen Feb 23, 2024

OriolAbril Mar 7, 2024

codecov bot commented Feb 23, 2024 •

edited

Loading

sethaxen commented Mar 3, 2024

OriolAbril left a comment

OriolAbril Mar 7, 2024

sethaxen Mar 7, 2024

OriolAbril Mar 11, 2024

sethaxen Mar 31, 2024

OriolAbril Apr 2, 2024

sethaxen Apr 3, 2024

OriolAbril Apr 3, 2024

sethaxen Apr 4, 2024

OriolAbril Mar 7, 2024

OriolAbril Mar 7, 2024

review-notebook-app bot commented Apr 4, 2024

sethaxen commented Apr 4, 2024

OriolAbril left a comment

OriolAbril left a comment

sethaxen commented Apr 4, 2024

OriolAbril commented Apr 4, 2024

OriolAbril commented Jun 10, 2024

OriolAbril commented Jun 11, 2024

		- Added arguments `band_prob`, `eval_points`, `rvs`, and `random_state` to `plot_ecdf` ([2316](https://github.com/arviz-devs/arviz/pull/2316))
		- Added rcParam `plot.band_prob` ([2316](https://github.com/arviz-devs/arviz/pull/2316))

Refactor plot_ecdf arguments #2316

Refactor plot_ecdf arguments #2316

Conversation

sethaxen commented Feb 23, 2024 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 23, 2024 • edited Loading

Codecov Report

sethaxen commented Mar 3, 2024

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

review-notebook-app bot commented Apr 4, 2024

sethaxen commented Apr 4, 2024

OriolAbril left a comment

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

sethaxen commented Apr 4, 2024

OriolAbril commented Apr 4, 2024

OriolAbril commented Jun 10, 2024

OriolAbril commented Jun 11, 2024

sethaxen commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading