arviz icon indicating copy to clipboard operation
arviz copied to clipboard

Refactor plot_ecdf arguments

Open sethaxen opened this issue 1 year ago • 2 comments

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
  • band_prob specifies band probability. plot.band_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 keywords:

  • pointwise is now confidence_bands="pointwise"
  • fpr is replaced with 1-band_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.

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/

sethaxen avatar Feb 23 '24 11:02 sethaxen

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.

codecov[bot] avatar Feb 23 '24 15:02 codecov[bot]

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

sethaxen avatar Mar 03 '24 07:03 sethaxen

Check out this pull request on  ReviewNB

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.

sethaxen avatar Apr 04 '24 13:04 sethaxen

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.

sethaxen avatar Apr 04 '24 18:04 sethaxen

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 avatar Apr 04 '24 18:04 OriolAbril

@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 avatar Jun 10 '24 16:06 OriolAbril

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

OriolAbril avatar Jun 11 '24 13:06 OriolAbril