stephenslab / susieR

R package for "sum of single effects" regression.
https://stephenslab.github.io/susieR
Other
176 stars 46 forks source link

`susie_suff_stat `: Arguments changed? #169

Open bschilder opened 2 years ago

bschilder commented 2 years ago

It seems the arguments to susie_suff_stat recently changed.

Could you add argument-specific deprecation warnings, or make the function backcompatible with the previous arguments?

Reprex

library(susieR)
data(N3finemapping)
attach(N3finemapping)

sumstats <- univariate_regression(X, Y[,1])
z_scores <- sumstats$betahat / sumstats$sebetahat
R <- cor(X)

fitted_bhat <- susieR::susie_suff_stat(
    bhat = sumstats$betahat,
    shat = sumstats$sebetahat,
    # maf = if("MAF" %in% colnames(dat)) dat$MAF else NULL,
    R = R,
    n = 1474097, # Number of samples/individuals in the dataset
    L = 5 # maximum number of non-zero effects 
)

Outputs

Using susieR v0.12.16:

Error in susie_func(bhat = dat$Effect, shat = dat$StdErr, maf = if ("MAF" %in%  : 
  unused arguments (bhat = dat$Effect, shat = dat$StdErr, R = base::data.matrix(LD_matrix))

Using susieR v0.11.92:

Runs without any error.

Best, Brian

pcarbo commented 2 years ago

@bschilder We recommend using the susie_rss interface instead for fine-mapping with summary statistics bhat, shat and n.

bschilder commented 2 years ago

Hi @pcarbo, I actually do use susie_rss in other scenarios, but I still think susie_suff_stat should be backcompatilbe to avoid breaking code that relies on it.

Changing argument names should be done very sparingly. If there is a strong reason to change the argument names, best practice in any package is to provide deprecation notices for at least 1 major version release. Packages like lifecycle facilitate this: https://lifecycle.r-lib.org/reference/deprecate_soft.html

This came up because echofinemap uses susie_suff_stat internally, and it wasn't clear from the errors why this function had suddenly stopped working.

pcarbo commented 2 years ago

@bschilder Fair enough, we will do our best to add notices, and we will leave this issue open as a reminder to us to take care of this. But please note that the R package is "in development" so we cannot guarantee backward compatibility.

@zouyuxin @gaow Is the error above expected in the new version of ssuieR? What informative message(s) would you suggest adding here?

bschilder commented 2 years ago

@pcarbo thank you for taking these concerns into consideration.

If the package is indeed still in development, I would argue that it should not be released onto CRAN yet: https://cran.r-project.org/web/packages/susieR/index.html

You can always use GitHub (or a branch in your GH repo) as the "development" version, and reserve CRAN releases for fully tested versions.

zouyuxin commented 2 years ago

@pcarbo yes. In <12 version, susie_suff_stat takes either 1.(bhat, shat, R, n, var_y) or 2. (XtX, Xty, yty, n). We compute 2 if 1 is provided. In version 12, we decided to simplify the interface and moved the computation to susie_rss, so susie_suff_stat takes XtX, Xty, yty, n only, and susie_rss is the main interface.

pcarbo commented 2 years ago

I pushed a change to provide an informative message for those trying to use an earlier version of the interface:

susie_suff_stat(bhat = 1,shat = 1,n = 100,R = 1)
# Error in susie_suff_stat(bhat = 1, shat = 1, n = 100, R = 1) :
# susie_suff_stat no longer accepts inputs bhat, shat, R or var_y; these inputs are now 
# accepted by susie_rss instead