Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to plotting functions #10

Open
pcarbo opened this issue Jun 28, 2024 · 12 comments
Open

Improvements to plotting functions #10

pcarbo opened this issue Jun 28, 2024 · 12 comments

Comments

@pcarbo
Copy link
Member

pcarbo commented Jun 28, 2024

@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:

plot_susiF
plot_susiF_pips
plot_susiF_effects

Also, please add a "plot" method which simply calls plot_susiF. See for example plot.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 and plot_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"):

plot_susiF  =  function (..., which.plots = c("both", "pips", "effects"), ...)

Let me know if you have any questions about these suggested changes.

@pcarbo
Copy link
Member Author

pcarbo commented Jun 28, 2024

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.

@pcarbo
Copy link
Member Author

pcarbo commented Jun 28, 2024

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).

@pcarbo
Copy link
Member Author

pcarbo commented Jun 28, 2024

You are using facet_grid for plotting mutliple effects?

@william-denault
Copy link
Collaborator

@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.

@pcarbo
Copy link
Member Author

pcarbo commented Jul 2, 2024

@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:

  • Sadly, "aes_string" is deprecated in ggplot2. (I found it useful, but sadly we cannot use it anymore.)

  • 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.

  • It looks like you are trying to add horizontal lines, but it doesn't show up in my effect plots?

  • You allow for additional arguments ("...") but it doesn't seem to be used anywhere? If so, you should remove it.

  • What does lfsr.curve = TRUE do? It doesn't seem to change my plot in the vignette example.

  • 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
  • I would expect these two lines to give me the same plot, but they gave me different plots:
plot_susiF_effect(fit,effect = 1:2)
plot_susiF_effect(fit,effect = 2:1)

@pcarbo
Copy link
Member Author

pcarbo commented Jul 2, 2024

Also note that "color" is the correct aesthetic in ggplot2, not "col".

@pcarbo
Copy link
Member Author

pcarbo commented Jul 2, 2024

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.

@william-denault
Copy link
Collaborator

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)
#Example using curves simulated under the Mixture normal per scale prior
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
#Running fSuSiE

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)
`

@pcarbo
Copy link
Member Author

pcarbo commented Jul 5, 2024

@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.

@pcarbo
Copy link
Member Author

pcarbo commented Jul 5, 2024

I added an option to plot_susiF_effect() to show the affected region, which is TRUE by default.

@pcarbo
Copy link
Member Author

pcarbo commented Jul 8, 2024

@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.

@pcarbo
Copy link
Member Author

pcarbo commented Jul 9, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants