Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added kwarg passing to pairwise_tests #352

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DavidALloyd
Copy link

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.

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

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

❗ 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.

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

@raphaelvallat
Copy link
Owner

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

Two things:

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

pt = pairwise_tests(

and when calculating the interaction:

for i, comb in enumerate(combs):

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

@DavidALloyd
Copy link
Author

DavidALloyd commented Apr 21, 2023

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'

def test_wilcoxon(self):

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

def test_ttest(self):

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

@raphaelvallat
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request 🚧 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants