Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix variance() in summarise_draws() #221

Merged
merged 6 commits into from
Jan 5, 2022
Merged

Fix variance() in summarise_draws() #221

merged 6 commits into from
Jan 5, 2022

Conversation

mjskay
Copy link
Collaborator

@mjskay mjskay commented Dec 31, 2021

Summary

This PR is mainly to fix #219 by adding a variance.draws_array() and variance.draws_matrix() implementation so that variance() works properly with summarise_draws(). For example this works correctly (and should continue to do so on future versions of {distributional}):

> summarise_draws(example_draws(), variance)
# A tibble: 10 x 2
   variable variance
   <chr>       <dbl>
 1 mu           11.6
 2 tau          12.8
 3 theta[1]     39.7
 4 theta[2]     21.5
 5 theta[3]     46.2
 6 theta[4]     24.2
 7 theta[5]     25.9
 8 theta[6]     26.6
 9 theta[7]     27.7
10 theta[8]     27.6

There are two other minor fixes I made while I was in here:

  • It adds support for casting from rvar to character vectors via vec_cast(x, character()) or as.character(x). This is mainly so that functions like paste() work as expected when passed rvars; e.g. this now works:
> x = rvar(rnorm(1000))
> paste("x =", x)
[1] "x = 0.01 ± 1"
  • It adds skip_if_not_installed("caret") to all tests of rstar() since that function requires {caret}, but {caret} is only in "Suggests".

Since these are all minor changes, once tests / coverage passes if I don't hear any objections in the next week or so I'll merge.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@codecov-commenter
Copy link

codecov-commenter commented Dec 31, 2021

Codecov Report

Merging #221 (46fa6df) into master (87a823b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 46fa6df differs from pull request most recent head 434df88. Consider uploading reports for the commit 434df88 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #221   +/-   ##
=======================================
  Coverage   94.32%   94.33%           
=======================================
  Files          41       41           
  Lines        2872     2875    +3     
=======================================
+ Hits         2709     2712    +3     
  Misses        163      163           
Impacted Files Coverage Δ
R/as_draws_array.R 95.00% <100.00%> (+0.05%) ⬆️
R/as_draws_matrix.R 97.67% <100.00%> (+0.02%) ⬆️
R/rvar-cast.R 91.76% <100.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87a823b...434df88. Read the comment docs.

@mitchelloharawild
Copy link

{distributional} revdep checks have finished, and it seems that this is the only change to worse. Once this PR makes it to CRAN, everything should be good to go for a {distributional} update. 👍 🤞

@paul-buerkner
Copy link
Collaborator

I am happy with the PR from my side. After you have merged this, I will prepare a new release of posterior to CRAN.

@mjskay
Copy link
Collaborator Author

mjskay commented Jan 5, 2022

Sounds good, thanks all! I fixed a minor test case failure on R < 4 and am merging now (the failure of macOS devel on github ci is spurious).

@mjskay mjskay merged commit 7ec5ae3 into master Jan 5, 2022
@mjskay mjskay deleted the variance branch January 5, 2022 09:19
@paul-buerkner
Copy link
Collaborator

Today I submitted the new version of posterior to CRAN and it should be released in a few days.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

variance(<draws_array>) method for changing default method in {distributional}
4 participants