stephenslab / mashr

An R package for multivariate adaptive shrinkage.
https://stephenslab.github.io/mashr
Other
88 stars 19 forks source link

Issue in cov_flash() function #115

Open fmorgante opened 1 year ago

fmorgante commented 1 year ago

Here is an error I have gotten with cov_flash. According to @willwerscheid, we need to handle the case where flash doesn't add any factors to the fit.

> library(mashr)
Loading required package: ashr
> dat_mash <- readRDS("test_data.rds")
> U_flash_default <- cov_flash(data=dat_mash, factors="default", tag="default",
+                              remove_singleton=TRUE, output_model=NULL)
Adding factor 1 to flash object...
Factor doesn't significantly increase objective and won't be added.
Wrapping up...
Done.
No factors have been added. Skipping backfit.
Error in sqrt(nrow(fl$F.pm)) : 
  non-numeric argument to mathematical function
pcarbo commented 1 year ago

@fmorgante With the fix, this script now gives:

Warning message:
In cov_flash(data = dat_mash, factors = "default", tag = "default",  :
  Flash fit does not have any factors
> U_flash_default
NULL

Does that seem like a reasonable behaviour @fmorgante @willwerscheid?

willwerscheid commented 1 year ago

Sure. Maybe we could make the warning message slightly more accessible, though? E.g., "Flash did not find any structure in the data, so no covariance matrices were created." I'm not sure exactly what the best phrasing would be -- it has been a while since I've done anything with mashr

fmorgante commented 1 year ago

Thank you @pcarbo! I agree with @willwerscheid that a more informative message would be preferable.

pcarbo commented 1 year ago

I updated the message slightly. (And in so doing realized that the docs were out-of-date, so I fixed that.) It is hard to say why flashier did not identify any factors; I think that is better documented in flashier, not in mashr. If you wanted a more helpful message I suppose we could give guidance to the user to improve the cov_flash call, e.g., increase the size of the data subset, or increase the number of PCs used.

fmorgante commented 1 year ago

Sounds good.