biometrician / abe

An R package for Augmented Backward Elimination
GNU General Public License v3.0
3 stars 0 forks source link

#32 exp.beta parameter in abe() #32

Closed biometrician closed 1 year ago

biometrician commented 1 year ago

After today's discussion, we came to the conclusion that exp.beta=TRUE for the linear model does not make much sense. Hence, we should delete this parameter again.

How to keep backward compatibility?

However, one recommended tau for linear, logistic and Cox does also not make much sense if the underlying criterion is different. After a discussion with Georg, the doubly standardized CIE for linear regression comes down to the partial correlation of X and Y. Hence, a threshold of 5% (0.05) is a lot. So we thought to reduce the default value for linear models to 0.01.

This would mean, that for logistic and Cox we have a default tau of 0.05 and for the linear model a default of 0.01. Is it possible to add a if() in the function call to abe(..., tau = ...)?

rokblagus commented 1 year ago

This could be done, although I am not a huge fan of this. Namely, in this case tau=NULL would have to be a default, and then we would need to have a clear description what the default is for any model. If we decide to do this, implementing this would be easy, but it would have to be done for abe and abe.resampling separately.

About using exp.beta=TRUE, we cannot simply delete this argument, unless it also does not make any sense at all for the other models, right? We can add a warning that if we have a LM model and the user wants to use exp.beta=TRUE, that this is probably not the best idea and that she/he should switch to exp.beta=FALSE.

biometrician commented 1 year ago

Hi, to summarize

for linear models we would like to use the exp.beta=FALSE change-in-estimate (CIE) criterion. for logistic or Cox models we would like to use the exp.beta=TRUE CIE criterion.

After a long discussion in the seminar and with Georg, we believe that it would not make much sense to change this. But we also realized if one uses a different CIE criterion, then the threshold should change. (We did not think about that back in 2014.) So as a quick recommendation, we came up with

for linear models a tau of 0.01 for logistic or Cox models a tau of 0.05.

(Of course, in the future this should be investigated in more detail. I have it on my list. :D But for now, it is a good start.)

My first idea was to add an if else statement in the function call. (e.g. abc <- function(x,y, type=if(x<2) {"minus"} else {"plus"}) { ... }. However, this does not look very nice in the function call.

Setting tau t0 NULL is the other obvious option. Also not optimal.

Currently, I have no idea, how we can implement this nicely in the package. I will think about it and also talk with Michael.

rokblagus commented 1 year ago

I think the only option is to put NULL as default for both in which case we automatically set them to the desired values (explaining this in the documentation). If they are specified to be non null, then we use them as they are provided by the user. Let me know, if I should make this change (it will have to be done in abe and abe.resampling). In this case we would first however have to merge Gregor's and my branch correctly! I opened a new issue about this.

biometrician commented 1 year ago

Hi Rok, I have finally time for the ABE project again.

This is a good idea. However, I just looked at the results using the new recommended tau for the linear model. I am not sure, if it is too restrictive. Before you change anything, maybe leave it is right now. And we discuss it together, when you are in Vienna.

I have just started to look at all the changes you and Gregor implemented. Let me finish looking through everything and then I will ask Gregor to merge the branches correctly.

biometrician commented 1 year ago

Hi Rok, after looking through some results, we would suggest to leave the current default value for \tau as it is at the moment. Before we adapt this recommendation we should investigate it in more detail.

biometrician commented 1 year ago

Hi Gregor, In abe and abe.resampling, we do not need the parameter exp.beta. The user should not be able to change this.

Is this possible: Delete the parameter in the function call? If a user using the old abe version uses it and exp.beta is not NULL, then the user should get a warning that this option is not longer available and will be overruled. (I think this should work if the, e.g., abe function has the arguments ... in the end. This should also be mentioned in the help file.

gregorsteiner commented 1 year ago

Done! This could potentially lead to problems if we decide to use the ... argument for something else in the future. We should keep this in mind.