Open pcarbo opened 3 months ago
Also, plot_susiF
should call the two other functions — right now it looks like plot_effect_susiF
and plot_susiF
have a lot of very similar or redundant code.
For plot_susiF_effects
, there should be an options to plot the effects for all CSs (which is the default, e.g., effect = "all"
) as well as the option to plot multiple selected CSs (e.g., effect = 1:2
).
You are using facet_grid for plotting mutliple effects?
@pcarbo Yes I do use facet_grid for plotting mutliple effects.
I have updated the repo have you suggested let me know if this looks good to you. Thank you very for editing the vignette.
@william-denault A made a few improvements to the effect plot, and now I call it "plot_susiF_effect" to make the function name consistent with the other plotting functions.
A few notes:
[x] Sadly, "aes_string" is deprecated in ggplot2. (I found it useful, but sadly we cannot use it anymore.)
[x] There seems to be quite a bit of redundant ggplot code in this function; I improved this a bit, but perhaps you could improve more.
[x] It looks like you are trying to add horizontal lines, but it doesn't show up in my effect plots?
[x] You allow for additional arguments ("...") but it doesn't seem to be used anywhere? If so, you should remove it.
[x] What does lfsr.curve = TRUE
do? It doesn't seem to change my plot in the vignette example.
[x] When I tried this I got an error:
plot_susiF_effect(fit,cred.band = FALSE,effect = "all")
# Error in rep(obj$outing_grid, indx_effect + 1) : invalid 'times' argument
plot_susiF_effect(fit,effect = 1:2)
plot_susiF_effect(fit,effect = 2:1)
Also note that "color" is the correct aesthetic in ggplot2, not "col".
I also made a few improvements to "plot_susiF_pip". (It was not an exported function, so I fixed that.)
Can you please add roxygen2 docs for this function?
Also, as before the additional arguments "..." seems to be unused? If so, I would remove it.
Hi @pcarbo ,
I have added these improvement and your other suggestions.
The argument lfsr.curve = TRUE
is for fsusie object that use the post_processing argument = "HMM" that I use in some other work. Essentially the output is the same but we postprocess the effect using an "ash-HMM" which allow to compute lfsr for each effect at each base pair. It is just another way to deal with the "incertainty"
You have a look at the example below
`library(ashr) library(wavethresh) set.seed(1)
rsnr <- 1 #expected root signal noise ratio N <- 100 #Number of individuals P <- 100 #Number of covariates/SNP pos1 <- 1 #Position of the causal covariate for effect 1 pos2 <- 5 #Position of the causal covariate for effect 2 lev_res <- 7#length of the molecular phenotype (2^lev_res) f1 <- simu_IBSS_per_level(lev_res )$sim_func#first effect f2 <- simu_IBSS_per_level(lev_res )$sim_func #second effect
G = matrix(sample(c(0, 1,2), size=N*P, replace=TRUE), nrow=N, ncol=P) #Genotype beta0 <- 0 beta1 <- 1 beta2 <- 1 noisy.data <- list()
for ( i in 1:N) { f1_obs <- f1 f2_obs <- f2 noise <- rnorm(length(f1), sd= (1/ rsnr ) var(f1)) noisy.data [[i]] <- beta1G[i,pos1]f1_obs + beta2G[i,pos2]f2_obs+ beta1G[i,15]*(f1_obs-f2_obs)+ noise
} noisy.data <- do.call(rbind, noisy.data)
Y <- noisy.data X <- G
out <- susiF(Y,X,L=3 , prior = 'mixture_normal_per_scale') out2 <- susiF(Y,X,L=3 , prior = 'mixture_normal_per_scale', post_processing = "HMM") plot_susiF_effect(out) plot_susiF_effect(out2) plot_susiF_effect(out2, lfsr = FALSE) `
@william-denault Thanks, the effect plot function is looking better now. I made a few additional improvements; see my comment above with the checkmarks. I also streamlined the roxygen2 docs by putting the documentation for all plotting functions into a single help page; this will simplify the documentation.
I will keep this Issue open as I review the other plotting functions and continue to make improvements.
I added an option to plot_susiF_effect() to show the affected region, which is TRUE by default.
@william-denault I updated the plotting functions a bit more; please take a look at the new interface, which is illustrated in the new vignette.
@william-denault Can you fix plot_susiF_effect()
so that the colors used always start with blue, then green, etc, regardless of how the "effect" argument is set? e.g., if I set effect = 7, the color should be the same as effect = 1. Does that make sense?
@william-denault I'd like to suggest a few changes to the plotting functions that I think would make the plotting interface more logical and easier to use. I believe that these changes should be straightforward to implement.
First, I suggest having three functions:
Also, please add a "plot" method which simply calls
plot_susiF
. See for exampleplot.ebnm
in the ebnm package for an example of how to do this.Second, the functions should not actually plot anything to the screen, but only return ggplot objects:
plot_susiF_pips
andplot_susiF_effects
should each return a single ggplot object.plot_susiF
should return a ggplot object or a list containing two ggplot objects. The output depends on an argument "which.plots" which looks like this (and will replace "pip_only"):Let me know if you have any questions about these suggested changes.