rvar indexing and casting improvements

[mjskay]

Summary

This PR supersedes #244 and closes #242, #243, and #246. Specifically, it:

• Adds an implementation of drop() for rvars.
• Supports the remaining modes of diag() for rvars (diag() for rvars #246).
• Uses more robust parsing for named indices in as_draws_rvars(), including nested use
  of [, like x[y[1],2] (as_draws_rvars() cannot parse indices with nested brackets #243).
• Allows 0-length rvars with ndraws() > 1 (issue with 0-sized rvars #242). e.g.

  > rvar(array(numeric(), dim = c(10,0)))
  rvar<10>[0] mean ± sd:
  NULL

• Ensures 0-length rvars can be cast to draws formats (issue with 0-sized rvars #242); e.g.

  > as_draws_matrix(rvar(array(numeric(), dim = c(10,0))))
  # A draws_matrix: 10 iterations, 1 chains, and 0 variables
      variable
  draw
    1 
    2 
    3 
    4 
    5 
    6 
    7 
    8 
    9 
    10

• Includes some miscellaneous improvements to testing/coverage for rvars.

Also worth pointing out is that this PR overrides some base functions (diag() and drop()) using S4 instead of S3 (hence the added methods dep). This is because those functions are overloaded as S4 generics in other packages folks might be using (particularly Matrix), so I think this is the best way to prevent weird incompatibilities if both packages are loaded.

Copyright and Licensing

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

• Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
• Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

[codecov-commenter]

Codecov Report

Merging #247 (237f795) into master (3b85c1b) will increase coverage by 0.65%.
The diff coverage is 100.00%.

❗ Current head 237f795 differs from pull request most recent head 4359aed. Consider uploading reports for the commit 4359aed to get more accurate results

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
+ Coverage   94.35%   95.00%   +0.65%     
==========================================
  Files          41       41              
  Lines        2886     2904      +18     
==========================================
+ Hits         2723     2759      +36     
+ Misses        163      145      -18     

Impacted Files	Coverage Δ
R/as_draws.R	85.00% <100.00%> (ø)
R/as_draws_rvars.R	100.00% <100.00%> (+5.95%)	⬆️
R/rvar-.R	96.69% <100.00%> (+1.20%)	⬆️
R/rvar-cast.R	91.76% <100.00%> (ø)
R/rvar-dim.R	100.00% <100.00%> (ø)
R/rvar-math.R	100.00% <100.00%> (+1.66%)	⬆️
R/rvar-rfun.R	100.00% <100.00%> (+8.57%)	⬆️
R/rvar-slice.R	100.00% <100.00%> (+2.38%)	⬆️
R/rvar-bind.R	98.55% <0.00%> (+1.44%)	⬆️
... and 4 more

[mjskay]

@wds15 I would be curious if you have feedback on this PR, as these issues were largely ones you raised. I believe the only outstanding thing to figure out is whether or not to make drop = TRUE the default (i.e. #245), but I think that will take a bit more thought and so can be a later PR.

[wds15]

Great to see this pr. What do you suggest to test it? Should I recode the example I posted previously?

[mjskay]

Sure! Yeah, hopefully the weird indices should parse properly now and you can at least do diag() without rdo(). I'm not sure in what context you were using 0-length rvars but hopefully that is fixed as well.

[wds15]

Ok, will do, the 0 length thing is triggered by a unit test of the OncoBayes2 package. Let me report back.

[mjskay]

This PR also fixes #248 now.

[wds15]

I tested the things I reported:

• 0 sized rvars
• diag for rvars
• having "[]" in index names

all good as it looks.

Just one thought:

It's not NULL, but rather 0 sized. Other than that the 0 sized rvar appears to behave fine.

I was not quite sure what to do about drop? The thing with drop is that I usually really want to control which index gets killed. This is why I often prefer adrop which allows me to control which dimension is being killed. So maybe add a adrop which works with rvars?

As I am going to rely with OncoBayes2 in its next version on posterior, I am going to hold that release until these updates have made it to CRAN. So I am looking forward to see it in a 1.2.3 release.

Thanks a lot for addressing these things quickly. Much appreciated.

[mjskay]

I tested the things I reported:

• 0 sized rvars
• diag for rvars
• having "[]" in index names

all good as it looks.

Awesome, thanks for taking a look!

It's not NULL, but rather 0 sized. Other than that the 0 sized rvar appears to behave fine.

Good point. Since the typical behavior on other zero-length objects seems to be to print the constructor that results in a zero-length object, I've changed the behavior to this:

rvar()
## rvar<1>[0] mean ± sd:
## rvar()

I was not quite sure what to do about drop? The thing with drop is that I usually really want to control which index gets killed. This is why I often prefer adrop which allows me to control which dimension is being killed. So maybe add a adrop which works with rvars?

Fortunately, because adrop() appears to be implemented in terms of dim() and dimnames(), which already work on rvar(), adrop() appears to already work on rvar()! Since this is desirable behavior, I added a test or two to make sure it continues to work in the future.

As I am going to rely with OncoBayes2 in its next version on posterior, I am going to hold that release until these updates have made it to CRAN. So I am looking forward to see it in a 1.2.3 release.

Cool!! Not sure when @paul-buerkner is planning a new release. Once this PR is in I plan to submit #236 shortly after that, which together should cover a bunch of outstanding rvar() things.

Thanks a lot for addressing these things quickly. Much appreciated.

Thanks you for taking the time to test things and write it up! It's very exciting to see it being put through its paces :)

[paul-buerkner]

I think we can schedule a new release flexibly. We have not many pressing issues that need to be resolved first (I think). If I schedule a release in the first week of Augst, would that work for this issue and for Sebastians package?

[wds15]

Works for me. Thanks!

[mjskay]

Works for me too. This PR is ready to be merged when you have a chance to look at it. After that I will submit #236 and I think that's all on my plate for the next release.

There are some bigger things I want to get to for a future release (like #149 and #208) but they will take a bit more thought and time.

[paul-buerkner]

Looks good to me. Thank you!