added show_non_significant flag to annotator #66
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #66 +/- ##
==========================================
- Coverage 97.04% 96.80% -0.25%
==========================================
Files 29 29
Lines 1898 1908 +10
==========================================
+ Hits 1842 1847 +5
- Misses 56 61 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Hello @sepro ,
Sorry I'm couldn't answer sooner.
Thanks a lot for the PR! It's a good start! We could also care about other use cases while we're at it, so I provided a few suggestions.
Let me know how far you want to go with this :-)
statannotations/Annotator.py
Outdated
| @@ -78,7 +78,7 @@ class Annotator: | |||
|
|
|||
| def __init__(self, ax, pairs, plot='boxplot', data=None, x=None, | |||
| y=None, hue=None, order=None, hue_order=None, | |||
| engine="seaborn", verbose=True, **plot_params): | |||
| engine="seaborn", verbose=True, show_non_significant=True, **plot_params): | |||
There was a problem hiding this comment.
I would rather have a hide_non_significant=False, the default being to show, and the action we want to enable is hide, if that makes sense.
There was a problem hiding this comment.
I don't have a strong opinion about this and it is an easy refactor.
statannotations/Annotator.py
Outdated
| ax_to_data=ax_to_data, | ||
| ann_list=ann_list, | ||
| orig_value_lim=orig_value_lim) | ||
| if self.show_non_significant or annotation.data.pvalue < 0.05: |
There was a problem hiding this comment.
I think we should first here check that isinstance(annotation.data, StatResult). Its a bit defensive but we can raise an error with a nice message to help users if they mixed that up.
Then, as the threshold is not always .05, and there can be multicomparison correction, how about using annotation.data.pvalue <= annotation.data.alpha if there is no correction method, or annotation.data.corrected_significance otherwise?
If we want to push further to increase readability, we would have a is_significant property on StatResult that would do this computation, and annotate here would simply check annotation.data.is_significant.
This has my preference but I could also refactor on top of your contribution.
There was a problem hiding this comment.
I like the approach you suggest (especially that there are no additional parameters to include somewhere), I'll give implementing it this way a shot !
There was a problem hiding this comment.
@sepro
Hello Sebastian,
Can I help with anything?
There was a problem hiding this comment.
Hi, it has been a bit of a busy month and didn't have the bandwidth to tackle this. It still is on my radar and something I would like to do, but if you can get to it faster your are obviously welcome to do so as well.
Significance is now checked based on the alpha and (corrected) p_value instead of hard coded.
|
#95 is the PR with the improved implementation, based on our discussion here. |
This is a proposed solution to discussion #50 with a single flag to show/hide non-significant issues.
@trevismd: I based this on the master branch only noticed later that PR should be based on the latest dev branch, though as merging is possible I hope this isn't an issue.