Repository: JuliaStats/HypothesisTests.jl
Pull request #243 (2021-09-10 14:02:06)

fix pvalue > 1 in  MannWhitneyUTest (#126)
Fix taken from [here](https://github.com/SurajGupta/r-source/blob/a28e609e72ed7c47f6ddfbb86c85279a0750f0b7/src/library/stats/R/wilcox.test.R#L76) in the R source code. 

Tests seem to fail for FischerExactTest for unrelated reasons. 



pdeffebach: Ready for a review @andreasnoack @nalimilan 
pdeffebach: I fixed all the ocurrences of `pvalue` in that file. `MannWhitneyUTest` dispatches to either the exact or approximate depending on some condition. 

I'm sure there are more cases of bad p-values but we can tackle those more slowly in a slew of smaller PRs as they are discovered. 
pdeffebach: Bumping this @andreasnoack. Can I get a review / merge?
andreasnoack: @devmotion Have your comments been addressed?
pdeffebach: > Can you add a test for the type stability of pvalue(::ApproximateMannWhitneyTest)? It can be provoked on the master branch with basically any standard example and you can just change pvalue to @inferred(pvalue(...)).

Added `@inferred` where possible, including fixing a bad inference in the `ApproximateMannWhitneyU` test. 

> Can you add a test for pvalue(::ApproximateMannWhitneyUTest; tail=:both) <= 1? Currently both tests just check the ExactMannWhitneyUTest. Unfortunately, the second example already works on the master branch for the approximate version so one needs different data to provoke a p-value > 1.

Added a test, which is really just testing inference. 
pdeffebach: Thanks, let me know if you have more comments. 
codecov-commenter: # [Codecov](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats) Report
> Merging [#243](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats) (dd95c42) into [master](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/commit/dd35ae0f1c08b3ae3558e1dada9fc57e41eff1aa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats) (dd35ae0) will **decrease** coverage by `11.58%`.
> The diff coverage is `66.66%`.

[![Impacted file tree graph](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/graphs/tree.svg?width=650&height=150&src=pr&token=ztSoTXYVhb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats)](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats)

```diff
@@             Coverage Diff             @@
##           master     #243       +/-   ##
===========================================
- Coverage   92.92%   81.33%   -11.59%     
===========================================
  Files          28       28               
  Lines        1639     1125      -514     
===========================================
- Hits         1523      915      -608     
- Misses        116      210       +94     
```


| [Impacted Files](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats) | Coverage Δ | |
|---|---|---|
| [src/mann\_whitney.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL21hbm5fd2hpdG5leS5qbA==) | `67.74% <66.66%> (-13.97%)` | :arrow_down: |
| [src/box\_test.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL2JveF90ZXN0Lmps) | `46.15% <0.00%> (-47.18%)` | :arrow_down: |
| [src/clark\_west.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL2NsYXJrX3dlc3Quamw=) | `62.50% <0.00%> (-37.50%)` | :arrow_down: |
| [src/jarque\_bera.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL2phcnF1ZV9iZXJhLmps) | `63.63% <0.00%> (-36.37%)` | :arrow_down: |
| [src/wald\_wolfowitz.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL3dhbGRfd29sZm93aXR6Lmps) | `64.28% <0.00%> (-35.72%)` | :arrow_down: |
| [src/breusch\_godfrey.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL2JyZXVzY2hfZ29kZnJleS5qbA==) | `64.28% <0.00%> (-35.72%)` | :arrow_down: |
| [src/bartlett.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL2JhcnRsZXR0Lmps) | `71.42% <0.00%> (-28.58%)` | :arrow_down: |
| [src/white.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL3doaXRlLmps) | `74.07% <0.00%> (-23.15%)` | :arrow_down: |
| [src/circular.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL2NpcmN1bGFyLmps) | `53.62% <0.00%> (-23.08%)` | :arrow_down: |
| [src/correlation.jl](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL2NvcnJlbGF0aW9uLmps) | `78.26% <0.00%> (-21.74%)` | :arrow_down: |
| ... and [19 more](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats). Last update [dd35ae0...dd95c42](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/243?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats).

devmotion: Seems Julia 1.7 broke the tests, probably due to the RNG changes. In general, equality checks of random results with fixed seeds seem very brittle and not reliable since different Julia versions are not guaranteed to yield the same random numbers (as happened now) - IMO ideally they should be replaced with some more stable tests. But of course this is not something you should have to deal with in this PR...
pdeffebach: @andreasnoack can this be merged? Test failures unrelated. 
andreasnoack: Let's get tests green before merging. I've opened https://github.com/JuliaStats/HypothesisTests.jl/pull/252 as a minimal PR to make tests green again.
pdeffebach: Will we merge #252 and then rebase?
pdeffebach: Now that #252 is merged, what is the next course of action?
devmotion: Update the PR so that tests pass, i.e., either merge the master branch or rebase the PR :slightly_smiling_face: 
pdeffebach: Okay I rebased

Then I changed the base branch back and forth on the github UI so the changes look good. I think we should run the workflow. 

Patch:
diff --git a/src/mann_whitney.jl b/src/mann_whitney.jl
index da1e959..df3a715 100644
--- a/src/mann_whitney.jl
+++ b/src/mann_whitney.jl
@@ -141,10 +141,11 @@ function pvalue(x::ExactMannWhitneyUTest; tail=:both)
         # cannot account for ties
         if tail == :both
             if x.U < x.nx * x.ny / 2
-                2 * pwilcox(x.U, x.nx, x.ny, true)
+                p = pwilcox(x.U, x.nx, x.ny, true)
             else
-                2 * pwilcox(x.U - 1, x.nx, x.ny, false)
+                p = pwilcox(x.U - 1, x.nx, x.ny, false)
             end
+            min(2 * p, 1.0)
         elseif tail == :left
             pwilcox(x.U, x.nx, x.ny, true)
         else # tail == :right
@@ -226,7 +227,8 @@ function pvalue(x::ApproximateMannWhitneyUTest; tail=:both)
     if x.mu == x.sigma == 0
         1.0
     elseif tail == :both
-        2 * ccdf(Normal(), abs(x.mu - 0.5 * sign(x.mu))/x.sigma)
+        p = 2 * ccdf(Normal(), abs(x.mu - 0.5 * sign(x.mu))/x.sigma)
+        min(p, 1.0)
     elseif tail == :left
         cdf(Normal(), (x.mu + 0.5)/x.sigma)
     else # tail == :right


Test patch:
diff --git a/test/mann_whitney.jl b/test/mann_whitney.jl
index 5076b35..0f61490 100644
--- a/test/mann_whitney.jl
+++ b/test/mann_whitney.jl
@@ -53,4 +53,25 @@ end
 @testset "Issue #39" begin
     @test abs(@inferred(pvalue(ExactMannWhitneyUTest(Float32[1:10;], Float32[2:11;]))) - 0.5096) <= 1e-4
 end
+
+@testset "Issue #126 pvalues above 1" begin
+	A = [1.34937, 1.75722,0.276514, 1.04546, 1.69085, 0.738085, 2.36313]
+	B = [2.62325, 1.16533, 1.1327, 0.728714]
+
+	m = ExactMannWhitneyUTest(A,B)
+	p = @inferred(pvalue(m; tail = :both))
+	@test p == 1
+
+	A = [12,10,7,6,3,1]
+	B = [11,9,8,5,4,2]
+
+	m = MannWhitneyUTest(A,B)
+	p = @inferred(pvalue(m; tail = :both))
+
+	@test p == 1
+
+	m = ApproximateMannWhitneyUTest(A, B)
+	p = @inferred(pvalue(m; tail = :both))
+end
+
 end