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

Adds text wrapping option to barplot, heatplot, dotplot, & ridgeplot #73

Merged
merged 3 commits into from
Oct 28, 2020

Conversation

RichardJActon
Copy link
Contributor

Adds the option label_wrap_width which defaults to 30 at the moment
This addresses the issue arrising from long terms for gene sets causing your graphs to be too small or sometimes even competely hidden as a result of very long names.

enrichplot:::barplot.enrichResult(ex, label_wrap_width = 100)

image

enrichplot:::barplot.enrichResult(ex, label_wrap_width = 20)

image

NB Underscores are removed from names to allow wrapping

@RichardJActon RichardJActon changed the title Adds text wrapping option to barplot, heatplot, dotplot, ridgeplot Adds text wrapping option to barplot, heatplot, dotplot, & ridgeplot Oct 22, 2020
@GuangchuangYu
Copy link
Member

It is a neat idea to have this feature.

However, there are two concerns:

  1. stringr is a huge dependency and I personally would not like to import this pkg.
  2. I believe a labeller parameter that accept a user defined function would be a better solution, (e.g. image_fun example provided in http://yulab-smu.top/treedata-book/ggimage-tips.html). In this way, not only str_wrap but also other effects on the axis label can be applied.

@GuangchuangYu
Copy link
Member

GuangchuangYu commented Oct 24, 2020

hadley also trying to create a replacement for stringr, see https://github.com/hadley/stringb.

After re-think this problem, I think maybe your approach for introducing a new parameter is more user friendly, especially for newbie.

But I really don't want to import stringr. HereI write a my_str_wrap function for you and maybe you can incorporate it and update the code accordingly.

my_str_wrap <- function(string, width) {
    x <- gregexpr(' ', string)
    vapply(seq_along(x), 
        FUN = function(i) {
            y <- x[[i]]
            n <- nchar(string[i])
            len <- (c(y,n) - c(0, y)) ## length + 1
            idx <- len > width
            j <- which(!idx)
            if (length(j) && max(j) == length(len)) {
                j <- j[-length(j)]
            } 
            if (length(j)) {
                idx[j] <- len[j] + len[j+1] > width
            }    
            idx <- idx[-length(idx)] ## length - 1
            start <- c(1, y[idx] + 1)
            end <- c(y[idx] - 1, n)
            words <- substring(string[i], start, end)
            paste0(words, collapse="\n")
        },
        FUN.VALUE = character(1)
    )
}

example:

> my_str_wrap(c("abcd efgh xyz bb", "aaaa aa bbcd"), 2)
[1] "abcd\nefgh\nxyz\nbb" "aaaa\naa\nbbcd"     
> 
> 
> stringr::str_wrap(c("abcd efgh xyz bb", "aaaa aa bbcd"), 2)
[1] "abcd\nefgh\nxyz\nbb" "aaaa\naa\nbbcd"     
> my_str_wrap(c("abcd efgh xyz bb", "aaaa aa bbcd"), 6)
[1] "abcd\nefgh\nxyz bb" "aaaa\naa\nbbcd"    
> stringr::str_wrap(c("abcd efgh xyz bb", "aaaa aa bbcd"), 6)
[1] "abcd\nefgh\nxyz bb" "aaaa\naa\nbbcd"    

@RichardJActon
Copy link
Contributor Author

Thank you for looking over my PR.
I understand the concern about a large dependency like stringr I will adjust my fork to use the the wrap function you provided.
I have a couple of questions.

  • Would the utilities.R file be your prefered location for the my_str_wrap function?
  • What do you think of my choice of a default wrap length of 30 characters? I was a little uncertain about this as it is probably a nice default going forward but previous users may have optimised outputs around the existing behaviour which could be mostly maintained simply by setting a higher default e.g. 100 characters.

I also see your point about greater flexibility from providing a user specifiable labeller function. Perhaps we could have the best of both worlds by replacing my label_wrap_width argument with label_format which if provided with a numeric value will simply string wrap by default and if provided with a function will instead set labels = user_defined_function() within the scale function?

@GuangchuangYu
Copy link
Member

  1. Yes.

  2. I think 30 is OK.

  3. label_format that supports both string width and user defined function is a neat idea and I would prefer this solution.

@GuangchuangYu GuangchuangYu merged commit 9644124 into YuLab-SMU:master Oct 28, 2020
@GuangchuangYu
Copy link
Member

thanks.

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

Successfully merging this pull request may close these issues.

2 participants