Repository: JuliaStats/HypothesisTests.jl
Pull request #113 (2017-09-06 12:37:39)

fix ApproximateMannWhitney for Float32
this mirrors the fix for ExactMannWhitney in https://github.com/JuliaStats/HypothesisTests.jl/issues/39

andreasnoack: It seems a bit odd to make the `struct` parametric when t effectively promotes to `Float64`. @simonster why did you make the change in https://github.com/JuliaStats/HypothesisTests.jl/commit/1c2d2c452c0f2385861f7c48ca23e6ef70cc29be the way you did?
codecov-commenter: # [Codecov](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/113?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats) Report
> Merging [#113](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/113?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats) (b5c8ec4) 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 **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/113/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/113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats)

```diff
@@           Coverage Diff           @@
##           master     #113   +/-   ##
=======================================
  Coverage   92.92%   92.92%           
=======================================
  Files          28       28           
  Lines        1639     1639           
=======================================
  Hits         1523     1523           
  Misses        116      116           
```


| [Impacted Files](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/113?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/113/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats#diff-c3JjL21hbm5fd2hpdG5leS5qbA==) | `81.70% <ø> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/113?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/113?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaStats). Last update [dd35ae0...b5c8ec4](https://codecov.io/gh/JuliaStats/HypothesisTests.jl/pull/113?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).

bjarthur: @nalimilan `ranks` is always Float64 because it's defined to be [here](https://github.com/JuliaStats/HypothesisTests.jl/blob/master/src/common.jl#L62)

@andreasnoack `median()` retains the type so the median field needs to be the same type as the input args

julia nightly is failing on the anderson_darling testset, which is unrelated.  these tests seem to be non-determinstic, as locally they pass sometimes and fail others.

can you please review again?  i think it's ready to merge.
bjarthur: bump.  @nalimilan approved previously, and i've just resolved the conflicts with master.  can we please merge?  thanks.
bjarthur: the only CI test that fails is anderson-darling for macOS 1.3.  this test passes for me locally on my mac using julia v1.3.  not sure what the difference is.  moreover, not sure how the changes proposed in this PR relate to anderson-darling.  lastly, why do we support 1.3 in HypothesisTests?  1.6 is the current LTS, and locally it passes there as well. could we just merge this PR?
ararslan: > the only CI test that fails is anderson-darling for macOS 1.3. this test passes for me locally on my mac using julia v1.3. not sure what the difference is. moreover, not sure how the changes proposed in this PR relate to anderson-darling.

Probably a flaky test, definitely unrelated to the PR

> why do we support 1.3 in HypothesisTests?

No one has submitted a PR to change the compatibility
ararslan: > i don't understand why the changes to type restrictions are needed either, but the test in this PR for Float32 fails without them

I checked out your branch, changed the method signature back to what it was, and ran tests, and they passed for me locally. 🤔
ararslan: Actually when I made the change back to what it was, I missed one particular parameter, which solves the mystery. 😄 Only the `mean::Float64` -> `median::Real` change is needed for the method signature. I've put that in a suggestion.

Patch:
diff --git a/src/mann_whitney.jl b/src/mann_whitney.jl
index df3a715..1bcd38b 100644
--- a/src/mann_whitney.jl
+++ b/src/mann_whitney.jl
@@ -165,11 +165,11 @@ end
 
 struct ApproximateMannWhitneyUTest{T<:Real} <: HypothesisTest
     U::Float64              # test statistic: Mann-Whitney-U statistic
-    ranks::Vector{T}        # ranks
+    ranks::Vector{Float64}  # ranks
     tie_adjustment::Float64 # adjustment for ties
     nx::Int                 # number of observations
     ny::Int
-    median::Float64         # sample median
+    median::T               # sample median
     mu::Float64             # normal approximation: mean
     sigma::Float64          # normal approximation: std
 end
@@ -200,7 +200,7 @@ where ``\\mathcal{T}`` is the set of the counts of tied values at each tied posi
 Implements: [`pvalue`](@ref)
 """
 function ApproximateMannWhitneyUTest(U::Real, ranks::AbstractVector{T},
-    tie_adjustment::Real, nx::Int, ny::Int, median::Float64) where T<:Real
+    tie_adjustment::Real, nx::Int, ny::Int, median::Real) where T<:Real
     mu = U - nx * ny / 2
     sigma = sqrt((nx * ny * (nx + ny + 1 - tie_adjustment /
         ((nx + ny) * (nx + ny - 1)))) / 12)


Test patch:
diff --git a/test/mann_whitney.jl b/test/mann_whitney.jl
index 0f61490..d883db1 100644
--- a/test/mann_whitney.jl
+++ b/test/mann_whitney.jl
@@ -54,6 +54,10 @@ end
     @test abs(@inferred(pvalue(ExactMannWhitneyUTest(Float32[1:10;], Float32[2:11;]))) - 0.5096) <= 1e-4
 end
 
+@testset "Issue #113" begin
+    @test abs(pvalue(ApproximateMannWhitneyUTest(Float32[1:10;], Float32[2:11;])) - 0.4948) <= 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]
@@ -73,5 +77,4 @@ end
 	m = ApproximateMannWhitneyUTest(A, B)
 	p = @inferred(pvalue(m; tail = :both))
 end
-
 end