pingouin icon indicating copy to clipboard operation
pingouin copied to clipboard

added kwarg passing to pairwise_tests

Open DavidALloyd opened this issue 1 year ago • 4 comments

Added ability to pass kwargs to downstream scipy functions using pairwise_tests. Previously did not have that ability, so changing things like the zero method or confidence weren't possible through pairwise_tests. pg.wilcoxon and pg.mwu already pass **kwargs downstream, so I simply modified pairwise_tests to accept **kwargs as a parameter and pass it as an argument when Wilcoxon, MWU, or ttest are called. Tested on example data and works without issue.

DavidALloyd avatar Mar 21 '23 19:03 DavidALloyd

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5c5f61a) 98.55% compared to head (f21ee10) 98.55%.

:exclamation: Current head f21ee10 differs from pull request most recent head a7a3698. Consider uploading reports for the commit a7a3698 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #352   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files          19       19           
  Lines        3390     3390           
  Branches      559      559           
=======================================
  Hits         3341     3341           
  Misses         26       26           
  Partials       23       23           
Impacted Files Coverage Δ
pingouin/pairwise.py 99.46% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Mar 21 '23 19:03 codecov[bot]

Thanks @DavidALloyd, great suggestion. I apologize about the delayed response.

Two things:

  1. The **kwargs should also be added to the recursive call

https://github.com/raphaelvallat/pingouin/blob/5c5f61a09ee4e49ed81716fa9485527092fcc6b0/pingouin/pairwise.py#L481

and when calculating the interaction:

https://github.com/raphaelvallat/pingouin/blob/5c5f61a09ee4e49ed81716fa9485527092fcc6b0/pingouin/pairwise.py#L540

  1. Could you please add unit tests for mwu, wilcoxon, and ttest?

raphaelvallat avatar Apr 16 '23 07:04 raphaelvallat

No worries @raphaelvallat, thanks for getting back! I can definitely add **kwargs to the other calls, but I have a bit of a dumb question though about the unit tests. I'm new to setting up tests like this, so my apologies if this is an ignorant question.

2. Could you please add unit tests for `mwu`, `wilcoxon`, and `ttest`?

test_nonparametryic.py already seems to contain tests for 'wilcoxon' and 'mwu'

https://github.com/raphaelvallat/pingouin/blob/5c5f61a09ee4e49ed81716fa9485527092fcc6b0/pingouin/tests/test_nonparametric.py#L63

https://github.com/raphaelvallat/pingouin/blob/5c5f61a09ee4e49ed81716fa9485527092fcc6b0/pingouin/tests/test_nonparametric.py#L86

and there's a test for 'ttest' in test_parametric.py

https://github.com/raphaelvallat/pingouin/blob/5c5f61a09ee4e49ed81716fa9485527092fcc6b0/pingouin/tests/test_parametric.py#L33

Was there something specific you wanted me to implement in the testing of pairwise_tests in test_pairwise.py?

DavidALloyd avatar Apr 21 '23 22:04 DavidALloyd

Hi @DavidALloyd!

My message was confusing, sorry about that. We want to add unit tests to test_pairwise_tests to ensure that the **kwargs work as expected. One simple way that doesn't require any ground-truth values from an external software (e..g JASP, JAMOVI, R, Matlab, etc) is to simply ensure that the output of pingouin.pairwise_tests is not the same when using a specific kwarg compared to the default. This needs to be tested for between/within and mixed design (with interaction). You can also just compare the output of pingouin.pairwise_tests (with only 2 groups) to the lower-level mwu, wilcoxon, and ttest functions when using a specific kwarg.

Does that make more sense?

Thanks!

raphaelvallat avatar Apr 26 '23 07:04 raphaelvallat