search

const-ae / ggsignif

Easily add significance brackets to your ggplots
https://const-ae.github.io/ggsignif/
GNU General Public License v3.0
593 stars 43 forks source link

New "extend_line" parameter. #70

Closed romanhaa closed 4 years ago

romanhaa commented 4 years ago

Hi Constantin,

I implemented a new extend_line parameter that allows the user to horizontally shorten or extend the bracket line that is drawn between the groups for each comparison. Negative values shorten it, positive values extend it. I did this because when slightly shortening the lines, one can draw several comparisons on the same height (y position) without them overlapping, which saves some space and can look like this:

ggplot(iris, aes(Species, Petal.Length)) +
geom_boxplot() +
geom_signif(
  comparisons = list(
    c('setosa','versicolor'),
    c('versicolor','virginica')
  ),
  extend_line = -0.01
) +
theme_bw()

image

I think it's quite useful and hope you think so too.

There are 4 commits because the first implementation I made failed when the order of groups was such that the bracket line is drawn backward, e.g. from group 3 to group 2. Then I added some safety checks and removed a log message that I had forgotten.

Cheers, Roman

const-ae commented 4 years ago

Hi Roman, thanks for opening a PR, that is a really cool feature and I like it a lot. I just took a very brief look at the code and it seems high quality.

Just one small question: is there a reason you decided to make the adjustments in draw_group() and not compute_group()? I don't want to say that it's wrong, just intuitively I thought that extend_line would be similar to step_increase and should be handled in the same function?

Best, Constantin

romanhaa commented 4 years ago

Oh yes, you're right. I was a bit in a rush because I needed this feature for some figures in my thesis so I must've missed this more obvious solution. Would you prefer if I move the code to the compute_group() function?

const-ae commented 4 years ago

Well, that would of course be amazing.

I also noticed that you put the new parameter in the last place of the geom_signif() signature https://github.com/const-ae/ggsignif/pull/70/files#diff-c9e114176372e051472aacb65491cb33R302. I think it would be more consistent to move just after the step_increase parameter.

romanhaa commented 4 years ago

Alright, I'll do that. I anyway don't think I would've been happy with this implementation now that you pointed how awkwardly placed the code is 😅 I'll close this PR and make a new one as soon as I have it ready.

romanhaa commented 4 years ago

I just gave it a try but got some doubts. Maybe I'm missing something obvious but the difficulty I see is that in compute_group() the x and xend positions are strings (to which I can't add a numeric), while in draw_group() the same variables are numeric so they can be easily modified (to shorten/extend the bracket line). Also, I think a rule would have to be implemented twice, depending on whether a list of comparisons is provided by the user or not. In order to implement it only once, maybe it's better to leave it in draw_group()?

const-ae commented 4 years ago

That is totally fine. That is exactly the kind of argument that means that it is much better idea to keep the code in draw_group().

In that case, I will review the code as it is right now and after that it should be good to merge :)

const-ae commented 4 years ago

Thank you so much again. I have merged your changes and will see that I make a new release including the feature soon :)

romanhaa commented 4 years ago

Thank you too! I'm happy that I could contribute.