Skip to content

Conversation

@jCalderTravis
Copy link
Contributor

Reference issue

Provides a test relevant for #11511

What does this implement/fix?

An existing code test compared two-tailed parametric and non-parametric t-tests. This pull request adds the corresponding code tests for one-tailed t-tests.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely replicates the failure!

___________________________ test_permutation_t_test ____________________________
mne/stats/tests/test_permutations.py:68: in test_permutation_t_test
    assert_allclose(p_values[0], p_values_scipy, rtol=1e-2)
/usr/share/miniconda3/envs/test/lib/python3.8/contextlib.py:75: in inner
    return func(*args, **kwds)
E   AssertionError: 
E   Not equal to tolerance rtol=0.01, atol=0
E   
E   Mismatched elements: 1 / 1 (100%)
E   Max absolute difference: 0.35283719
E   Max relative difference: 1.01524173
E    x: array(0.700377)
E    y: array(0.34754)

Just pushing a little commit that will help show which tail is failing (maybe both one-sided?)

@larsoner
Copy link
Member

Oh, GitHub won't let me push the commit... feel free to make the suggested change and push @jCalderTravis !

jCalderTravis and others added 2 commits March 21, 2023 10:54
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@jCalderTravis
Copy link
Contributor Author

Thanks @larsoner for your suggestion. I have added this in, and now we get a test for each tail:

FAILED mne/stats/tests/test_permutations.py::test_permutation_t_test_tail[less--1] - AssertionError:
FAILED mne/stats/tests/test_permutations.py::test_permutation_t_test_tail[greater-1] - AssertionError:

@jCalderTravis
Copy link
Contributor Author

@larsoner would you be happy to merge this test? Thank you.

@larsoner
Copy link
Member

You need to run black on your file to get the style checks to be happy

If one of them is expected to fail you'll need to mark it as xfail (we never want red / failing tests merged into main). So you can do something like:

@pytest.mark.parametrize('whatever', [
    x,
    y,
    pytest.param(z, marks=pytest.mark.xfail(reason='Bug in permutations'),
])

@jCalderTravis
Copy link
Contributor Author

jCalderTravis commented May 24, 2023

Thanks for your comments @larsoner. I incorporated xfail as you suggested.

@jCalderTravis
Copy link
Contributor Author

Also now formatted with black.

@larsoner larsoner merged commit 52a42e2 into mne-tools:main May 24, 2023
@welcome
Copy link

welcome bot commented May 24, 2023

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@larsoner
Copy link
Member

Thanks @jCalderTravis !

@larsoner
Copy link
Member

Whoops I didn't realize this was your first PR to MNE-Python! Could you open another quick PR to add this change to doc/changes/latest.inc (maybe under the BUG section) with the :newcontrib: role along with name/url in doc/changes/names.inc? Want to make sure you get credit for this

larsoner pushed a commit that referenced this pull request May 26, 2023
Co-authored-by: Mathieu Scheltienne <mathieu.scheltienne@gmail.com>
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 23, 2023
* upstream/main: (24 commits)
  Allow int-like as ID of make_fixed_length_events (mne-tools#11748)
  Easycap-M43 montage (mne-tools#11744)
  ENH: Create a Calibrations class for eyetracking data (mne-tools#11719)
  Fix alphabetical order in overview/people.rst, fix sphinx formatting in docstrings and set verbose to keyword-only (mne-tools#11745)
  Add Mathieu Scheltienne to MNE-Python Steering Council (mne-tools#11741)
  removed requirement for curv.*h files to create Brain object (mne-tools#11704)
  [BUG] Fix mne.viz.Brain.add_volume_labels matrix ordering bug (mne-tools#11730)
  Fix installer links (mne-tools#11729)
  MAINT: Update for PyVista deprecation (mne-tools#11727)
  MAINT: Update roadmap (mne-tools#11724)
  MAINT: Update download link [skip azp] [skip cirrus] [skip actions]
  fix case for chpi_info[1] == None (mne-tools#11714)
  Add cmap argument for mne.viz.utils.plot_sensors (mne-tools#11720)
  BUG: Fix one more PySide6 bug (mne-tools#11723)
  MAINT: Fix PySide6 and PyVista compat (mne-tools#11721)
  MRG: If _check_fname() cannot find a file, display the path in quotation marks to help spot accidental trailing spaces (mne-tools#11718)
  Add "array-like" to `_validate_type()` (mne-tools#11713)
  MAINT: Avoid problematic PySide6 (mne-tools#11715)
  Fix installer links (mne-tools#11709)
  Updating change log after PR mne-tools#11575 (mne-tools#11707)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants