const-ae / ggsignif

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

ggcompare:Implement layer-level P value correction. #143

Open HMU-WH opened 2 weeks ago

HMU-WH commented 2 weeks ago

I found that ggsignif has never perfectly resolved the issue of P-value adjustment across panels, and ggpubr::stat_compare_means also has this problem. At the same time, both have deficiencies in adapting to faceting.

To address this, I created ggcompare to fill the gaps in P-value adjustment and adaptability to faceting in both. Its primary function is to focus on mean difference testing.

Due to the different operational logic with ggproto objects, I chose not to submit a PR to optimize on the basis of ggsignif, but rather to refactor. However, my main inspiration still comes from ggsignif.

const-ae commented 2 weeks ago

Hi Hao,

that looks very cool. I am a bit confused about the code in your README, where do you define the logic of e.g. StatCompare?

Best, Constantin

HMU-WH commented 2 weeks ago

Hi Hao,

that looks very cool. I am a bit confused about the code in your README, where do you define the logic of e.g. StatCompare?

Best, Constantin

I did not explicitly provide the source code for StatCompare and GeomBracket in ggcompare because, in my view, these are just two ggproto objects. According to my usual coding habits, I load them as LazyData, directly copying them from my personal toolkit. Another reason is that my English proficiency is not high, and my source code contains a large number of non-English comments, so I did not copy the source code of StatCompare over.

You can view the source code of the corresponding components in R using the following method:

StatCompare$compute_layer
StatCompare$compute_panel
const-ae commented 2 weeks ago

Thanks for the explanation.

I did not explicitly provide the source code for StatCompare and GeomBracket in ggcompare because, in my view, these are just two ggproto objects

Well, it is a bit worrying to load some random binary files from the internet, as those could do anything to my computer. I don't want to suggest that you are doing anything problematic; it's just not good practice and will stop some people from installing your package (for more details see rdaradar post by Bob Rudis).

Another reason is that my English proficiency is not high, and my source code contains a large number of non-English comments, so I did not copy the source code of StatCompare over.

I can totally empathize with the feeling that it is scary for other people to look at your code. However, I would like to encourage you to make the code easily viewable before installation. This will help anybody (for example me :) ) who wants to learn how you managed to do the p-value adjustment across facets.

HMU-WH commented 2 weeks ago

Thanks for the explanation.

I did not explicitly provide the source code for StatCompare and GeomBracket in ggcompare because, in my view, these are just two ggproto objects

Well, it is a bit worrying to load some random binary files from the internet, as those could do anything to my computer. I don't want to suggest that you are doing anything problematic; it's just not good practice and will stop some people from installing your package (for more details see rdaradar post by Bob Rudis).

Another reason is that my English proficiency is not high, and my source code contains a large number of non-English comments, so I did not copy the source code of StatCompare over.

I can totally empathize with the feeling that it is scary for other people to look at your code. However, I would like to encourage you to make the code easily viewable before installation. This will help anybody (for example me :) ) who wants to learn how you managed to do the p-value adjustment across facets.

Thank you for your suggestion.

Now you can see the specific implementation process here.

const-ae commented 2 weeks ago

That's great :)

And congrats on the nice code. It is quite readable (even without comments) and your approach to do the calculation in compute_layer is a better choice than my way to do it in compute_group!

Two small things I notice skimming your code:

HMU-WH commented 2 weeks ago

you should rather use ggplot2:::parse_safe

I did not notice the situation you mentioned in the documentation for getFromNamespace. Using the ":::" form can prevent passing R CMD CHECK, while using getFromNamespacedoes not have this issue. Moreover, unless the method parse_safe is removed or changed in ggplot2 (which is unlikely), I believe this usage remains safe.

x |> f(_)

Yes, this is a format supported only in newer versions of R, but at least for now, I do not plan to modify it unless there are users who must use an older version of R.