pitakakariki / simr

Power Analysis of Generalised Linear Mixed Models by Simulation
68 stars 19 forks source link

Stop when test is `fixed` and `xname` does not exist in model #218

Closed pablobernabeu closed 3 years ago

pablobernabeu commented 3 years ago

Hello,

I would like to suggest a feature for powerCurve and powerSim, to stop these functions when the test in use is fixed, and the effect entered (i.e., xname) does not exist in the model. This feature would avoid unnecessary computation, and confusion regarding the results with 0% power and 'subscript out of bounds'.

I have tried to implement it in powerCurve.R by adding the following code around the top of the script, below the definition of the arguments.

    # [Work in progress] If the test is `fixed`, and `xname` does not exist in the model, stop with error.
    if(substr(test, 1, 5) == 'fixed') {
        # Function below extracts the effect from `fixed('')` or `fixed("")`, checks whether it exists in `fit`, 
        # and if it does not exist, the function stops with an error.
        if(gsub("^fixed\\(.(.+).\\)$", "\\1", test) %in% unique(names(fixef(fit))) == FALSE) {
            stop("Please double-check the name of the effect in the model, especially the order of the variables in interactions.")
        }
    }

Unfortunately, I get the error cannot coerce type 'closure' to vector of type 'character', caused by my use of the test argument in the text-processing functions substr() and gsub().

I hope this attempt can help implement the feature at some point. Perhaps someone would like to suggest some ideas, or to have a go themselves.

Thanks

pitakakariki commented 3 years ago

I'm working on setting some dedicated time aside next financial year for some overdue maintenance. This is definitely high on the list.

There are a number of things that need to be improved with fixed — I've been collecting them here: https://github.com/pitakakariki/simr/issues/122

I was going to make the fix in testLibrary.R but I think you're right that the check should be earlier. The code for the checks would need to be shared by powerSim and powerCurve though. At the moment I think I'll be building it into wrapTest, and change powerCurve to call wrapTest earlier.

pitakakariki commented 3 years ago

Note re your error message - you probably wanted something like deparse(substitute(test)[[1]]) == "fixed". Then later noquote(deparse(substitute(test)[[2]])).

That said it's probably not good to be using that kind of if statement (e.g. it will miss simr::fixed). When I get to this I think I'll put the checks in fixed itself, with the object passed through via wrapTest. That will also let me add similar checks to the other tests.

pablobernabeu commented 3 years ago

Thank you very much, Peter, for your explanations, and for all your work on this great package.