Add location and sign to KS test result by iosonofabio · Pull Request #17062 · scipy/scipy

iosonofabio · 2022-09-20T23:04:25Z

Reference issue

Aims to close #9389

What does this implement/fix?

Moves the result of Kolmogorov-Smirnov tests (1 or 2 samples) to a "tuple bunch" with two additional attributes:

statistic_location indicating at what value of the stochastic variable (approximately) the maximal distance in CDF is reached
statistic_sign, which is only nonempty for alternative="two-sided", indicating whether the first distribution (for 2samp)/the empirical distribution (for 1samp) was above or below the second/theoretical distribution at the statistic_location. This is a cheap way to determine not only whether the distros are different, but also if they are, in what direction.

Additional information

Matt and I quickly discussed part 1, the statistic_location. I later realized having part 2 i.e. the sign is equally important for me and perhaps for other folks, and is a cheap addition in terms of performance.

I haven't adapted the part of the docstring that represents doctests yet, happy to do so but perhaps it'd be useful if one of the maintainers took a quick look the the draft PR first so I know if there are any major issues.

Thank you for your time and patience

mdhaber · 2022-09-20T23:17:58Z

@tupui @tirthasheshpatel @chrisb83 If you agree that this looks like a good idea, I can review this. There was a lot of user support in the original issue gh-9389, and they seem happy to have it added to the existing function, so I think it's fine to add these attributes rather than create a new function.

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

mdhaber · 2022-09-21T05:52:29Z

OK, looks like there's just one real failure now:

scipy/scipy/stats/tests/test_mstats_basic.py

Lines 1877 to 1882 in b5a8052

    
           def test_nametuples_agree(self): 
        
               result = stats.kstest([1, 2], [3, 4]) 
        
               assert_(isinstance(result, stats._stats_py.KstestResult)) 
        
               result2 = stats._stats_py.Ks_2sampResult(result.statistic, result.pvalue) 
        
               assert_(isinstance(result2, stats._stats_py.Ks_2sampResult)) 
        
               assert_equal(result, result2)

This Ks_2sampResult:

scipy/scipy/stats/_stats_py.py

Line 7489 in 2e5883e

Ks_2sampResult = KstestResult

is no longer used anywhere, so I'd suggest just deleting the definition of it and deleting the test. That looks like it will fix the remaining failures.

tupui · 2022-09-21T09:08:53Z

I am +0.5. I personally would go with a separate function. We don't really have a lot of extra info in other tests. Instead we could have a general function to get the distance between 2 distributions (our API is slim there) and it would make perfect sense to have the location there.

mdhaber · 2022-09-22T00:53:49Z

One part of my hesitation with adding a separate function is that if we were to add a function for the statistic used in each of our hypothesis tests, we'd double the number of functions in stats. I doubt we want to head in that direction, and I don't think the KS statistic is so special that is it deserves a separate function while the rest do not.

Also, it makes me think of Halton/Hammersley - those are actually different (Hammersley will take an additional input for the offset of the linear sweep), yet we're talking about combining them. Here, we'd duplicate the function that computes the statistic so that there are two nearly identical functions, the only difference being that one version has the location of the maximum difference and the other has a p-value?

We've started to add more outputs to hypothesis tests already (e.g. df and confidence interval methods in t-test), so I think this fits. We still need to be selective about what we add to keep the API clean, but there seems to be a fair amount of support for adding this somewhere, so I think an attribute is fine.

You're right, though - we could have a function for comparing two empirical CDFs, or an empirical CDF vs a theoretical CDF, with different distance metrics. The latter would be goodness_of_fit, basically, and the former wod be the equivalent function for two samples.

tupui · 2022-09-22T11:05:58Z

I agree that we don't want to duplicate the API with a location function for all tests.

Here it's just that for this specific test we do something very generic and useful otherwise which is to compute the distance between CDFs.

I am fine doing both as well. Adding now an output for the location and maybe latter have a separate distance function.

iosonofabio · 2022-09-22T11:25:25Z

We could make an optional input argument called compute_Pvalue=True that skips the last part and is used just to compute the max distance and its location. Would that be a useful compromise in your opinion or just a mess? 😏

mdhaber · 2022-09-22T21:09:42Z

Mmm I'm more hesitant to add inputs to functions than outputs, and even more hesitant to add an input that instructs the function to add an output : )

It sounds like we have an agreement here - what @tupui said.

Adding now an output for the location and maybe latter have a separate [CDF] distance function.

mdhaber · 2022-09-24T04:14:24Z

So to sum up @iosonofabio we can continue with this PR. Please consider the adjustments recommended above and I can take a second pass. I'd also suggest adding a unit test in test_stats.py. a good test would be to confirm that the statistic and p-value result object can still be unpacked and match the attributes, and also that the new attributes are present and appropriate (if possible).

iosonofabio · 2022-09-24T04:32:31Z

No problem, I'll get it done as soon as I get back to Oz (haven't slept in a while now...)

Thank you!

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

mdhaber · 2022-10-01T22:32:10Z

I saw you posted some new commits - are you ready for this to be reviewed again, or was there more to do?

iosonofabio · 2022-10-02T09:18:24Z

Thanks @mdhaber - I was just overwhelmed in the last few days.

I have now added decent test functions, improved docstrings a little, and changed a piece of logic that you suggested and I believe was incorrect (please double check if you have time!).

If it passes CI, it is ready for you to take a look, thank you! Otherwise I'll just fix the errors first.

mdhaber

Thanks @iosonofabio. The implementation looks perfect. My only comments are about the documentation and the tests.

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

mdhaber · 2022-10-07T00:47:34Z

+            Value of `x` corresponding with the KS statistic; i.e., the 
+            distance between the hypothesized cumulative distribution function
+            and the empirical distribution function is greatest at this
+            location.


Is this what you wanted statistic_location for ks_1samp to be?

This is what I expected it to be, but it's not that (as written). Instead, it is the CDF at which the statistic is measured.

Oops, that explains why the ks_2samp tests are fine but I couldn't get the one sample tests to work... Will fix tonight, thank you

mdhaber · 2022-10-07T01:00:44Z

Please see iosonofabio#1.

MAINT: stats: suggestions for "Add location and sign to KS test result"

iosonofabio · 2022-10-10T22:22:19Z

Hey @mdhaber - thank you again for your PR. I merged it and added similar tests for the one-sample KS test.

I think this is ready to go in if you guys approve. Also happy to make other changes or, if you prefer, feel free to push commits into this PR directly 😄

iosonofabio · 2022-10-11T07:00:45Z

Sorry Matt, was taken in by some stuff! Let me check test results properly and I'll ask for review at that point. A day or two. Thank you

…

On Sun, Oct 2, 2022, at 09:32, Matt Haberland wrote: I saw you posted some new commits - did you want this to be reviewed again? — Reply to this email directly, view it on GitHub <#17062 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJFEAHHXXCRNU6F7M7XSMLWBC3XLANCNFSM6AAAAAAQRQZWHQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

iosonofabio · 2022-10-14T20:22:21Z

Thanks @mdhaber - implemented all requested changes. No further edits necessary AFAICT.

mdhaber

It looks like the implementation in ks_1samp has been corrected and documentation is better. The main comments are about the tests. It seems that what needs to be tested is essentially the same for kstest, ks_1samp, and ks_2samp, but what is actually tested differs. Try to simplify the changes. We should see some symmetry in the testing (whetever is done for ks_1samp has a parallel for ks_2ssamp, and somehow we see that kstest is the same as those two).

iosonofabio · 2022-10-15T09:45:58Z

@mdhaber Thanks for the comment. I disagree that symmetry is essential. The 1-sample KS test is against a continuous distribution, whereas the 2-sample test is against a discretization thereof, which incurs all kinds of aliasing effects. I have done my best to try and satisfy the conflicting requirements of simplicity and theoretical clarity. As of now, I unfortunately have no time left for this PR. I'm not trying to be rude, but I really have no more time for this.

Given we are at the finish line, I would be grateful if you could push any changes that you think would improve the PR, symmetry or otherwise, and merge when you think it's good enough.

Thank you

mdhaber · 2022-10-15T14:46:23Z

The comment about some symmetry was resolved by the two recent commits. (It really crossed paths with the first of them, even though it shows up afterwards.)

mdhaber · 2022-10-16T02:44:23Z

                        res3 = stats.kstest(xm, y, alternative=alternative, mode=mode)
                        assert_equal(np.asarray(res1), np.asarray(res3))

-    def test_nametuples_agree(self):


Rather than modifying this test, we can just remove it. It doesn't belong in test_mstats_basic.py if it's about stats objects anyway, and Ks_2sampResult is no longer used either in stats or mstats, so I think it's safe to remove this.

mdhaber

Approved, but I'd like a second opinion before merging, since I had a lot to do with the documentation and tests.

tirthasheshpatel

Looks good to me too! Thanks @iosonofabio @mdhaber! I initially thought that a new function would be better but, looking at this PR, it seems like either work just fine (with respect to API design choice). Since I haven't gone through all the reviews and comments on this PR, I would leave the decision to merge on @mdhaber.

mdhaber · 2022-10-23T01:48:13Z

Thanks @tirthasheshpatel! @tupui I'm going to merge now but if you have a chance for a quick glance, let me know if there is anything you'd like to change before the release and I'll open another PR.

iosonofabio · 2022-10-23T01:52:53Z

Thanks @mdhaber and everyone for your hard work and guidance!

Add location and sign to KS test result

2b1b089

tylerjereddy added scipy.stats enhancement A new feature or improvement labels Sep 20, 2022

mdhaber reviewed Sep 21, 2022

View reviewed changes

Comment thread scipy/stats/_stats_py.py Outdated

Comment thread scipy/stats/_stats_py.py Outdated

make_tuple_bunch syntax and lines too long

d06ae0a

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

mdhaber reviewed Sep 21, 2022

View reviewed changes

iosonofabio and others added 2 commits September 24, 2022 14:33

Slightly better logic from Matt

e3d8a30

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

Fixes suggested and test: missing one test still

b7ae2bb

Docstrings, tests, one test helper, and one logic mistake

909ddf1

One line is too long ;-)

7a47ed2

iosonofabio force-pushed the issue_9389 branch from cc7eadf to 7a47ed2 Compare October 2, 2022 20:07

mdhaber reviewed Oct 3, 2022

View reviewed changes

Improve wording on some docstrings

1ad08f7

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

mdhaber reviewed Oct 7, 2022

View reviewed changes

mdhaber added 2 commits October 6, 2022 17:49

DOC: stats: additional clarifications of statistic_location/sign

44d3477

TST: stats: revert test; add test that is easier (for me) to understand

64dd533

mdhaber mentioned this pull request Oct 7, 2022

MAINT: stats: suggestions for "Add location and sign to KS test result" iosonofabio/scipy#1

Merged

Bugfix on ks_1samp, better tests

b3c8b64

iosonofabio added 2 commits October 8, 2022 08:46

Merge pull request #1 from mdhaber/issue_9389_revisions

5e26ce5

MAINT: stats: suggestions for "Add location and sign to KS test result"

Tests for ks_1samp a la Matt

3d06417

iosonofabio marked this pull request as ready for review October 7, 2022 22:48