Issue 267 ecdf plots with confidence bands

[TeemuSailynoja]

A reworked implementation of the ECDF plots mentioned in issue #267 to replace my earlier pull request.

Main inclusions:
mcmc_rank_ecdf: rank ecdf plot with confidence bands for assessing if two or more chains sample the same distribution.
ppc_pit_ecdf and ppc_pit_ecdf_grouped: PIT ECDF plot with confidence bands to assess if y and yrep contain samples from the same distribution.

Both functions allow computation of the confidence bands and interpolation of these bands from precomputed values.

[paul-buerkner]

This PR looks already quite good. I like the code structure and readability. However, I think there are not yet any unit tests and both the internal and user-facing documentation is a bit short at times and may need some fixes as described in my comments.

[paul-buerkner]

This looks good, thank you! The only remaining issue is the storage of a couple of .svg files which I am not sure should really be tracked by git.

[paul-buerkner]

Thanks! Looks good to me. Checks are now in progress.

[TeemuSailynoja]

There was one final bug introduced in a merge and I had apparently run the test without loading the merged files in. I have now fixed that in the ppc_pit_ecdf functions and all the tests should pass.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 87.34940% with 42 lines in your changes missing coverage. Please review.

Project coverage is 97.72%. Comparing base (0f931bc) to head (f5c0dc3).
Report is 150 commits behind head on master.

Files with missing lines	Patch %	Lines
R/helpers-ppc.R	76.76%	33 Missing ⚠️
R/ppc-distributions.R	95.20%	6 Missing ⚠️
R/mcmc-traces.R	95.38%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage   98.47%   97.72%   -0.76%     
==========================================
  Files          35       35              
  Lines        4661     4969     +308     
==========================================
+ Hits         4590     4856     +266     
- Misses         71      113      +42     

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

[paul-buerkner]

There are still some checks failing. Can you take a look and fix them?

[avehtari]

This PR has been approved a long time again, but some checks are failing. Based on a quick lookm at least one failure is due to a typo, so it would be good to check the failure messages

[TeemuSailynoja]

I fixed the typo causing the error in testing.

[paul-buerkner]

Thanks! Checks are running now and I will merge as soon as they pass.

[paul-buerkner]

Still fails. Do you see why?

[avehtari]

Windows:

Error: Error: processing vignette 'visual-mcmc-diagnostics.Rmd' failed with diagnostics:
invalid connection

Ubuntu:

  Error in get_interpolation_values(N, K, L, prob) : 
    No precomputed values to interpolate from for sample length of 93.
  Please use a subsample of length 100 or larger, or consider setting 'interpolate_adj' = FALSE.
  Calls: ppc_pit_ecdf_grouped ... adjust_gamma -> interpolate_gamma -> get_interpolation_values
  Execution halted

[TeemuSailynoja]

I fixed the Ubuntu issue. This windows one is on a seemingly unrelated Vignette, and I wasn't able to parse from the error message, what exactly is failing in the processing of the Rmd file.

[paul-buerkner]

Okay, let's try again.

[TeemuSailynoja]

Silly mistake of failing to remember to commit the updated Rd files after changing the documentation in the .R files. Still, the windows test seems to fail while compiling a Stan model from a Vignette.

[paul-buerkner]

All the checks that actually run (and don't fail because of unrelated reasons) have passed and the PR also passed for me locally. Accordingly, I will merge this PR now. Thank you for your work!

[jgabry]

I'm finally doing another bayesplot release so I will include this in the release. Thanks again @TeemuSailynoja for implementing this and @paul-buerkner and @avehtari for reviewing!

I noticed a few complaints from R cmd check still

❯ checking dependencies in R code ... NOTE
  There are ::: calls to the package's namespace in its code. A package
    almost never needs to use ::: for its own objects:
    ‘gamma_adj’

❯ checking R code for possible problems ... NOTE
  mcmc_rank_ecdf: no visible binding for global variable ‘parameter’
  mcmc_rank_ecdf: no visible binding for global variable ‘chain’
  ppc_pit_ecdf: no visible binding for global variable ‘y_id’
  ppc_pit_ecdf_grouped: no visible binding for global variable ‘y_id’
  Undefined global functions or variables:
    chain parameter y_id

but I think I have fixed these in #291.

[TeemuSailynoja]

Hi @jgabry, last two edits I would like to include for the ECDF plots would be to

1. change the default behaviour to plot the regular ECDF instead of ECDF-difference.
   • Users are likely to be more familiar with this plot even if with large samples the difference shows a more dynamic range.
2. change the default behaviour from always attempting to use interpolateion to compute the envelope to only interpolating for large samples.
   • The speed-up for small samples is minor and there is a chance of interpolation failing due to lack of precomputed data.

I already have these edits ready, if you think they can make it to the release. The question is, should I commit them to a separate branch, here, or to patch-for-cran for review?