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

make map_signif_level more flexible #52

Merged
merged 6 commits into from
Feb 20, 2019
Merged

make map_signif_level more flexible #52

merged 6 commits into from
Feb 20, 2019

Conversation

ilia-kats
Copy link
Contributor

map_signif_level can now take a user-supplied function to format the p-value. Also, this pull-request changes the default threshold for printing exact p-values to .Machine$double.eps (should still be 2e-16 on most computers)

@const-ae
Copy link
Owner

Hi, thank you very much for the pull request. I am currently travelling, but will take a look next week.

draw_key_point leads to weird errors when trying to use manual
xmin/xmax/annotation with data grouped by multiple aesthetics
Copy link
Owner

@const-ae const-ae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ilia, thanks again a lot for the PR. I have added some comments to the code and it would be great if you could comment on them.
Best, Constantin

}else{
if(is.numeric(p_value)){
if(p_value < .Machine$double.eps){
sprintf("p < %.2e", .Machine$double.eps)
}else{
as.character(sprrintf("p = %.2g"), p_value)
as.character(sprintf("p = %.2g"), p_value)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think printing just the number without the "p = " in front is the better default, can you change that back?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. I just thought it would be more consistent with "p < 2e-16".

@@ -215,7 +215,7 @@ GeomSignif <- ggplot2::ggproto("GeomSignif", ggplot2::Geom,
required_aes = c("x", "xend", "y", "yend", "annotation"),
default_aes = ggplot2::aes(shape = 19, colour = "black", textsize = 3.88, angle = 0, hjust = 0.5,
vjust = 0, alpha = NA, family = "", fontface = 1, lineheight = 1.2, linetype=1, size=0.5),
draw_key = ggplot2::draw_key_point,
draw_key = function(...){grid::nullGrob()},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this change do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely prevents drawing of a legend key for the ggsignif layer. In my case, I was trying to use ggsignif with geom_sina from ggforce and kept getting weird errors until I made this change.

@@ -145,6 +145,7 @@ StatSignif <- ggplot2::ggproto("StatSignif", ggplot2::Stat,
#' @param map_signif_level boolean value, if the p-value are directly written as annotation or asterisks are used instead.
#' Alternatively one can provide a named numeric vector to create custom mappings from p-values to annotation:
#' For example: c("***"=0.001, "**"=0.01, "*"=0.05)
#' Alternatively, one can provide a function that takes a numeric argument (the p-value) and returns a string
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you maybe also add an example that demonstrates how to use the custom function, for example to show how to print the p-values as "p = 0.03"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@const-ae const-ae merged commit 47363ea into const-ae:master Feb 20, 2019
@const-ae
Copy link
Owner

Thank you again, I have merged the PR and will submit the updated package to CRAN soon.

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

Successfully merging this pull request may close these issues.

2 participants