fix pvalue > 1 in MannWhitneyUTest (#126) #243

pdeffebach · 2021-09-10T14:02:06Z

Fix taken from here in the R source code.

Tests seem to fail for FischerExactTest for unrelated reasons.

src/mann_whitney.jl

pdeffebach · 2021-09-10T14:27:20Z

Ready for a review @andreasnoack @nalimilan

nalimilan

Thanks. It's kind of weird that we have to clamp the value like that, but that's less surprising if other software do that. Anyway PValue throws an error for values above 1 and we probably shouldn't change that.

Do we need the same fix for other Mann-Whitney tests?

src/mann_whitney.jl

test/mann_whitney.jl

nalimilan · 2021-09-10T14:33:44Z

test/mann_whitney.jl

 	@test abs(pvalue(ExactMannWhitneyUTest(Float32[1:10;], Float32[2:11;])) - 0.5096) <= 1e-4
 end
+
+@testset "Issue #126" begin


Say what this issue is about?

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

pdeffebach · 2021-09-10T14:49:31Z

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.

src/mann_whitney.jl

test/mann_whitney.jl

pdeffebach · 2021-10-26T01:29:45Z

Thanks for the review. This can be merged.

pdeffebach · 2021-11-09T20:57:58Z

Bumping this @andreasnoack. Can I get a review / merge?

andreasnoack · 2021-11-09T21:06:48Z

@devmotion Have your comments been addressed?

devmotion

It seems the type instability was fixed 👍 I think two things should be improved:

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(...)).
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.

devmotion · 2021-11-09T21:18:55Z

test/mann_whitney.jl

+	A = [12,10,7,6,3,1]
+	B = [11,9,8,5,4,2]
+
+	m = MannWhitneyUTest(A,B)


This will also be an ExactMannWhitneyUTest, so the changes for the p-value and type stability in the ApproximateWhitneyUTest are not tested.

I wanted to suggest changing this to

m = ApproximateMannWhitneyUTest(A,B)

but unfortunately the p-value is already 1 for this example so it can only be used for testing type stability of pvalue.

I can't find inputs to get the p-value above 1 here. But I did fix an inference issue, which is nice.

pdeffebach · 2021-11-29T20:44:28Z

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 · 2021-11-29T20:45:01Z

Thanks, let me know if you have more comments.

devmotion

Looks good to me now, thank you @pdeffebach for taking into account all comments!

codecov-commenter · 2021-12-03T13:52:46Z

Codecov Report

Merging #243 (56df1db) into master (dad954e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #243   +/-   ##
=======================================
  Coverage   93.64%   93.65%           
=======================================
  Files          28       28           
  Lines        1715     1717    +2     
=======================================
+ Hits         1606     1608    +2     
  Misses        109      109

Impacted Files	Coverage Δ
src/mann_whitney.jl	`87.80% <100.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dad954e...56df1db. Read the comment docs.

devmotion · 2021-12-03T16:11:25Z

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 · 2021-12-28T19:38:40Z

@andreasnoack can this be merged? Test failures unrelated.

andreasnoack · 2021-12-28T21:49:23Z

Let's get tests green before merging. I've opened #252 as a minimal PR to make tests green again.

pdeffebach · 2021-12-28T23:46:12Z

Will we merge #252 and then rebase?

pdeffebach · 2022-03-03T22:14:36Z

Now that #252 is merged, what is the next course of action?

devmotion · 2022-03-03T22:27:40Z

Update the PR so that tests pass, i.e., either merge the master branch or rebase the PR 🙂

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

…pothesisTests.jl into fix_pvalue_too_big

pdeffebach · 2022-03-04T20:21:18Z

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.

nalimilan

@devmotion Merge?

devmotion · 2022-03-09T16:48:29Z

Thank you @pdeffebach 🙂

fix MannWhitneyUTest

4fde74c

pdeffebach commented Sep 10, 2021

View reviewed changes

src/mann_whitney.jl Outdated Show resolved Hide resolved

src/mann_whitney.jl Outdated Show resolved Hide resolved

use float

94ead63

devmotion reviewed Sep 10, 2021

View reviewed changes

src/mann_whitney.jl Show resolved Hide resolved

nalimilan linked an issue Sep 10, 2021 that may be closed by this pull request

p val > 1 in ExactMannWhitneyUTest #126

Closed

nalimilan reviewed Sep 10, 2021

View reviewed changes

pdeffebach and others added 2 commits September 10, 2021 10:46

Apply suggestions from code review

86b1559

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

describe issue

ea846fa

nalimilan requested a review from andreasnoack September 10, 2021 15:15

nalimilan approved these changes Sep 10, 2021

View reviewed changes

nalimilan requested a review from devmotion October 2, 2021 18:55

devmotion reviewed Oct 2, 2021

View reviewed changes

review

04a4553

add 1.0

75663b2

devmotion requested changes Nov 9, 2021

View reviewed changes

add ending if

dd95c42

devmotion approved these changes Dec 3, 2021

View reviewed changes

pdeffebach and others added 8 commits March 4, 2022 12:26

fix MannWhitneyUTest

886959c

use float

895797c

Apply suggestions from code review

f3094c0

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

describe issue

ca4a35e

review

af1b698

add 1.0

068a3a3

add ending if

0cffe12

Merge branch 'fix_pvalue_too_big' of https://github.com/pdeffebach/Hy…

f528374

…pothesisTests.jl into fix_pvalue_too_big

pdeffebach changed the base branch from master to dw/type_instability March 4, 2022 20:20

pdeffebach changed the base branch from dw/type_instability to master March 4, 2022 20:20

Merge branch 'master' into fix_pvalue_too_big

56df1db

nalimilan approved these changes Mar 9, 2022

View reviewed changes

devmotion approved these changes Mar 9, 2022

View reviewed changes

devmotion merged commit ab97607 into JuliaStats:master Mar 9, 2022

fix pvalue > 1 in MannWhitneyUTest (#126) #243

fix pvalue > 1 in MannWhitneyUTest (#126) #243

Uh oh!

Conversation

pdeffebach commented Sep 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdeffebach commented Sep 10, 2021

Uh oh!

nalimilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nalimilan Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

pdeffebach Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

pdeffebach commented Sep 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdeffebach commented Oct 26, 2021

Uh oh!

pdeffebach commented Nov 9, 2021

Uh oh!

andreasnoack commented Nov 9, 2021

Uh oh!

devmotion left a comment

Choose a reason for hiding this comment

Uh oh!

devmotion Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

pdeffebach Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

pdeffebach commented Nov 29, 2021

Uh oh!

pdeffebach commented Nov 29, 2021

Uh oh!

devmotion left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devmotion commented Dec 3, 2021

Uh oh!

pdeffebach commented Dec 28, 2021

Uh oh!

andreasnoack commented Dec 28, 2021

Uh oh!

pdeffebach commented Dec 28, 2021

Uh oh!

pdeffebach commented Mar 3, 2022

Uh oh!

devmotion commented Mar 3, 2022

Uh oh!

pdeffebach commented Mar 4, 2022

Uh oh!

nalimilan left a comment

Choose a reason for hiding this comment

Uh oh!

devmotion commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov-commenter commented Dec 3, 2021 •

edited

Loading