Use StatsAPI.pvalue + StatsAPI.HypothesisTest
#297
Conversation
Co-authored-by: Alex Arslan <ararslan@comcast.net>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #297 +/- ##
==========================================
- Coverage 93.78% 93.75% -0.04%
==========================================
Files 28 28
Lines 1739 1729 -10
==========================================
- Hits 1631 1621 -10
Misses 108 108
☔ View full report in Codecov by Sentry. |
|
Tests pass with the latest release of Distributions. |
| @@ -25,76 +25,23 @@ | |||
| module HypothesisTests | |||
|
|
|||
| using Statistics, Random, LinearAlgebra | |||
| using Distributions, Roots, StatsBase | |||
There was a problem hiding this comment.
Initially it seemed as if we could get rid of the StatsBase dependency completely. It turned out that this is not the case but the five functions loaded below are needed + StatsBase.PValue for printing. Nevertheless, as with the imports of Combinatorics and Rmath in the lines below, explicitly importing these five functions seemed a bit clearer.
There was a problem hiding this comment.
I don't personally find listing out every symbol to be particularly clarifying or useful but I know not everyone shares that opinion. :) In this case I'd prefer not to change the StatsBase bit as part of this PR as it seems like an unrelated change but I don't feel too strongly.
There was a problem hiding this comment.
I don't have a completely strong opinion on this matter either. I wasn't 100% sure what changes you had in mind exactly, but I reverted this line and some related changes below. Is it better now?
There was a problem hiding this comment.
It looks ✨ a m a z i n g ✨, apologies for the pedantry and for the delay, this got lost amidst a bunch of other notifications
|
Bump 🙂 |
Fixes #290.
The PR requires JuliaStats/Distributions.jl#1719. Keeping these definitions in HypothesisTests would be type piracy when
pvalueis not owned by HypothesisTests anymore. Alternatively, the definitions forUnivariateDistributions could be moved to an internal_pvaluefunction but IMO they seem more generally useful, so I moved them to Distributions.