Extend normal outcome functions to two-sided decisions

[danielinteractive]

normal outcome part of #27

Summary of the updates:

• decision1S and decision2S: allows lower.tail to have as many elements as pc to allow the specification of two-sided decision boundaries (mixed lower.tail elements, i.e. some specifying "lower" and some "upper") to capture intermediate result scenarios.
• decision1S_boundary.normMix and decision2S_boundary.normMix: for two-sided decision boundaries, return a list of lower_than and higher_than critical boundaries (calculated as before, and separately).
• oc1S.normMix: for two-sided decision boundaries, calculates the probability to be in-between the lower_than and higher_than bounds, as returned by decision1S_boundary.normMix
• oc2S.normMix: for two-sided decision boundaries, defines the internal freq function accordingly to use the lower_than and higher_than bounds as produced from the given theta1 and theta2 values.
• pos1S.normMix: similar update here as in oc1S.normMix
• pos2S.normMix similar update to oc2S.normMix

Notes:

• The source contributed will be licensed under the GPL3.
• This will afterwards be extended to the other outcome types (beta, gamma distributions) in separate PRs such that each PR is not too large. The package should not be released until this is completed, such that it is consistent for the user.

[weberse2]

Overall this looks like a good direction. Thanks. There are a number of comments I had during review.

Two general remarks:

• the code must be contributed under the GPLv3 and will stay under the GPLv3
• Please use checkmate for most stuff... in particular if multiple
  conditions are checked on a single object. Checkmate produces far
  better error messages by default from my experience. No need to revise
  the entire package for this, but whenever you introduce some assert,
  consider the checkmate variant to check most stuff at once.

Also, please consider to revise the following pattern:

  lower.tail <- attr(decision, "lower.tail")
  if (length(lower.tail) > 1) {
    use_lower <- which(lower.tail)
    use_upper <- which(!lower.tail)
    assert_true(length(use_lower) > 0 && length(use_upper) > 0)
    dec_lower <- decision[use_lower]
    crit_lower <- decision1S_boundary.normMix(
      prior,
      n,
      dec_lower,
      sigma,
      eps
    )
    dec_upper <- decision[use_upper]
    crit_upper <- decision1S_boundary.normMix(
      prior,
      n,
      dec_upper,
      sigma,
      eps
    )

with something where we put this logic into the decision function
itself? So when setting up the decision function we check if it is
two-sided. Then we make that an attribute of the decision function so
that it knows that is is one/two sided. The decision object itself
could then also have a upper and lower decision object as an attribute
which are themselves one-sided in each direction.

Then the above would become

num_sides <- attr(decision, "sides")
if(num_sides != 1) {
  crit_lower <- decision1S_boundary.normMix(
      prior,
      n,
      attr(decision, "lower"),
      sigma,
      eps
    )
  crit_upper <- decision1S_boundary.normMix(
      prior,
      n,
      attr(decision, "upper"),
      sigma,
      eps
    )
}

And as the code needs quite often to know if it is two sided or one
sided we should go one further and have an internal function

.is_one_sided <- function(decision) {
  return TRUE / FALSE depending on whatever the thing is
}

Then the code gets much easier to follow along.

Finally, I would like to merge this onto a develop branch first. As
this work is not yet complete once this normal case is mixed, I would
not like it on the main branch to be merged. The main branch should
stay clean from half-baked features. Best practice is to have main be
the released thing, which I did not follow in the past, but with
staged feature inclusion, we need this.

Could you also update the NEWS file indicating that this feature is in
progress?

[danielinteractive]

Thanks a lot @weberse2 for your thoughtful review. I have implemented the suggested pattern change (with a slight variation, using an S3 class instead of merely an attribute to differentiate two-sided decision functions), as well as addressed the minor comments.

I agree (as stated in the PR description already) that the code will be contributed under GPLv3.

I recommend to

1. first merge the "Air formatting" PR Set up Air configuration and formatting #29
2. agree on the changes in this PR
3. I will then redirect this PR towards the develop branch
4. run checks, approve, merge then to develop

[weberse2]

Just minor stuff now. I am a bit worried on snapshot tests, see the comment below.

I would also appreciate to make the doc for users sound less technical - whatever class something has is possibly confusing to many users.

The class for a 2sided case is neat. I was wondering if one could even assign decision1S_1/2sided, decision1S as classes.. but maybe that's overkill and we can change it to this approach later easily?

I also noticed that the SBC tests have aged too much so that the gold runs are now too old. I can handle that.

Ah.. what would be nice for users is to have a small example of the new functionality in the examples. Some examples are already long... still a brief one would be useful, I think.

Getting close to merge.

[weberse2]

Can u please add return here in each case explicitly?

[danielinteractive]

Sure, done (just fyi https://style.tidyverse.org/functions.html#return discourages explicit returns when they are not needed)

[weberse2]

since there is no lower/upper attribute, one would get null when requesting these. Wouldn't it make more sense to have lower/upper attribute being defined for whatever the 1S is for... the other one should be set to NULL, which probably does not need defintion.

[danielinteractive]

I understand, that would also be possible. So far I was trying to be fully backwards compatible with this enhancement, i.e. I did not want to change the current one-sided objects or structure. Therefore I did not try to add attributes here or change the overall class structure for one-sided. If you think that is not important, then I can try to make the one-sided and two-sided more consistent with each other (i.e. closer to the more general two-sided)

[weberse2]

I have not used snapshot tests myself. These were not a thing when I worked on this. My worry when I see this here, is that the enormous precision here is super fragile over time. Maybe I need to read a bit how this test works in this case? Could you point me where I need to read up to understand this. As long as the test is robust and not prone to irrelevant floating point business, I am fine with it.

[danielinteractive]

The general intro is here: https://testthat.r-lib.org/articles/snapshotting.html
Specifically I am using expect_snapshot_value, see https://testthat.r-lib.org/reference/expect_snapshot_value.html, which tolerates relative numerical differences up to sqrt(.Machine$double.eps) (we could also add a little helper which has a larger tolerance if we run into issues) Also, on CRAN checks these snapshot tests are skipped.

[weberse2]

I don't think that users care about the class. So this should read just two-sided decision function... unless we switch to the logic I suggested to have this lower/upper being defined for one-sided functions as well, which I would actually prefer? Or is there a good reason not to have it consistent?

[danielinteractive]

changed - please see my other comment re: consistency vs. backwards compatibility question.

[danielinteractive]

Thanks @weberse2!

• I added a couple of examples now where I thought it fits well, let me know if you would like to have also examples for oc2S and the pos1S and pos2S functions.
• Please see my question about not changing the one-sided structures vs. more consistency between one-sided and two-sided.