Refactored ks_twosample to take in two slices instead of a mutable owned Vec<f64>

[AshrafIbrahim03]

Through some discussion in #405 , it seemed like refactoring ks_twosample was needed. Basically just changed the function signature from:

pub fn ks_twosample(
    mut data1: Vec<f64>,
    mut data2: Vec<f64>,
    method: KSTwoSampleAlternativeMethod,
    nan_policy: NaNPolicy,
) -> Result<(f64, f64), KSTestError>

to

pub fn ks_twosample(
    data1: &[f64],
    data2: &[f64],
    method: KSTwoSampleAlternativeMethod,
    nan_policy: NaNPolicy,
) -> Result<(f64, f64), KSTestError>

This is more in line with what's in other files in the same folder.

[day01]

@AshrafIbrahim03 did you verify tests with all targets n features?

[AshrafIbrahim03]

I did not. I just reread through the contributing section of the README to see if it details how to run those, but I don't see it. How can I do that?

[day01]

probably you find out:
cargo test

Thanks, the call sites compile now. I checked the latest commit but three KS tests still fail. currently order is wrong. are you sure it should be like that? may you add some context to Pr ?

[AshrafIbrahim03]

Yes, just saw the last three tests fail, pushing a fix for it now!

Some more context: basically we were talking about a contribution I could make in #405 , and it seemed like changing the ks sampling function to take a &[f64] instead of a mut Vec<f64> would be more in line with the other files, anderson_darling and chi_square.

I also noticed the ks one sample function that I should probably bundle in this PR too!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.82%. Comparing base (ad9676b) to head (0f070d0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #406   +/-   ##
=======================================
  Coverage   94.82%   94.82%           
=======================================
  Files          61       61           
  Lines       13539    13539           
=======================================
  Hits        12838    12838           
  Misses        701      701           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[YeungOnion]

I realize I steered you astray. I think it's better to let the caller opt into copying data. Ideally, we could obtain an iterator over the "sorted" data borrowing, but I don't know if that's easy to express, or we just verify order along with nan policy with an iterator argument.

Thoughts as a user?

[day01]

If I need to retain the input data, I can clone it explicitly, otherwise the function can consume and sort it without hidden copies

[YeungOnion]

@AshrafIbrahim03 that means we should go the other way, convert those that accept slices to instead own as Vec for data we sort in place.

It would be great if there were a way to get an iterator that's element-wise sorted, because then the API expresses we need each value in order. It's incidental that we happen to sort the data ourselves to access that ordering of the values efficiently. Maybe we'll defer that for larger datasets that need to be streamed.