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

TO DISCUSS #82

Open
CharlotteMicheloud opened this issue Jan 18, 2023 · 0 comments
Open

TO DISCUSS #82

CharlotteMicheloud opened this issue Jan 18, 2023 · 0 comments

Comments

@CharlotteMicheloud
Copy link
Collaborator

CharlotteMicheloud commented Jan 18, 2023

  • Sample size based on the relative effect size d. Do we keep it? It currently does not work for type = "controlled"

  • Documentation: currently "level" is documented as "level for replication success", which is not correct.

  • Internal naming of function. "alphaS" should be called "gamma". We should change it in the functions.

  • pSceptical(zo = 7, zr = 7, c = 2, type = "controlled") returns 0, numerical problems

  • Writing new tests (type = "controlled", other functions such as levelSceptical, etc)

=========================================================================================

General propositions

  • Should we use functions to test the arguments instead of just stopifnot?

    • We could throw better error/warning messages
    • This ensures that we allow the same inputs everywhere. Now we have just a bunch of stopifnot clauses copied all over the place.
    • Error messages are pretty bad now because stopifnot returns uninformative errors:
      • pReplicate(po = 0, c = 1, alternative = "two.sided")
      • Error in (function (p, alternative = c("two.sided", "one.sided", "less", : 0 < p is not TRUE
  • We have function checks but they are often repeated many times. Maybe we should have a distinction between internal and exported functions.

    • For example: z2p_int() which does all the transformations but no input checks and then an exported function z2p() which checks arguments and then calls z2p_int.
    • In combination with the idea above, this could lead to much more modular code and much less repetition in the the code base.

Specific points

  • Some issues with edge cases in conversionHelpers.R

    • See e.g. p2z(1, "one.sided") which returns -Inf
    • This leads to problems in other functions as well
      • With alpha = 1 in thresholdIntrinsic? (We do not allow alpha = 0)
      • ci2p(lower = 5, upper = 5, conf.level = 0.95, ratio = TRUE, alternative = "two.sided") throws an error. However, the error is not caught in the argument checks of ci2p. It is, however, caused by ci2z(lower = 5, upper = 5, conf.level = 0.95, ratio = TRUE) which evaluates to Inf which is then caught in the input checks of z2p, which uses the previous result as argument z = Inf.
  • I do not know why we export certain functions. We usually export the vectorized function but in some cases we also export the non-vectorized function.

    • exported
      • .z2p_
      • .p2z_
      • .powerSignificance_
      • .sampleSizeReplicationSuccess_
      • .pSceptical_
    • not exported
      • .sampleSizeSignificance_
  • Argument alpha of function pvalueBound has no input checks ensuring it is between 0 and 1

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

No branches or pull requests

1 participant