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

TestCorrelation::test_corr fails on some platforms #195

Closed
Tracked by #236
musicinmybrain opened this issue Sep 13, 2021 · 3 comments
Closed
Tracked by #236

TestCorrelation::test_corr fails on some platforms #195

musicinmybrain opened this issue Sep 13, 2021 · 3 comments
Assignees
Labels
docs/testing:book: Documentation and unit testing

Comments

@musicinmybrain
Copy link
Contributor

I’m helping to update the python-pingouin package in Fedora Linux. One test, TestCorrelation::test_corr, is failing on some platforms because what should theoretically be a perfect unity correlation is coming out two or three ulps short:

        # Compare BF10 with JASP
        df = read_dataset('pairwise_corr')
        stats = corr(df['Neuroticism'], df['Extraversion'])
        assert np.isclose(1 / float(stats['BF10'].to_numpy()), 1.478e-13)
        # Perfect correlation, CI and power should be 1, BF should be Inf
        stats = corr(x, x)
>       assert stats.at['pearson', 'r'] == 1
E       AssertionError: assert 0.9999999999999997 == 1

This happens when the tests are executed on an aarch64, ppc64le, or s390x platform. (On ppc64le and s390x, the correlation is 0.9999999999999998.)

What do you think? Does this look like this a bug to be fixed somewhere, or should the assertion be loosened to permit a value slightly less than one due to platform-dependent rounding?

@raphaelvallat raphaelvallat added the docs/testing:book: Documentation and unit testing label Sep 15, 2021
@raphaelvallat raphaelvallat self-assigned this Sep 15, 2021
@raphaelvallat
Copy link
Owner

Hi @musicinmybrain,

Interesting — thanks for opening the issue and helping with this! I think it is definitely a platform-dependent rounding error so we should use np.isclose(stats.at['pearson', 'r'], 1) instead of the strict ==.

Raphael

@musicinmybrain
Copy link
Contributor Author

Makes sense to me. You could, if you like, add an additional test for stats.at['pearson', 'r'] <= 1', since a high-quality implementation shouldn’t be producing correlations outside [-1, 1] no matter how the rounding happens to work out.

@raphaelvallat raphaelvallat mentioned this issue Feb 12, 2022
12 tasks
@raphaelvallat
Copy link
Owner

Done in 7f5f0cc

See PR v0.5.1: #236

raphaelvallat added a commit that referenced this issue Feb 20, 2022
* Flake8

* Explicit error when y is an empty list in pg.ttest

#222

* Add keyword arguments in homoscedasticity function

#218

* Bugfix rm_anova and mixed_anova changed the dtypes of categorical columns + added observed=True to all groupby

#224

* Update version number in init and setup

* Use np.isclose for test_pearson == 1

#195

* Coverage for try..except scipy fallback

* Fix set_option for pandas 1.4

* Upgraded dependencies for seaborn and statsmodels

* Added Jarque-Bera test in pg.normality

#216

* Coverage scipy import error

* Use pd.concat instead of frame.append to avoid FutureWarning

* Remove add_categories(inplace=True) to avoid FutureWarning

* GH Discussions instead of Gitter

* Minor doc fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs/testing:book: Documentation and unit testing
Projects
None yet
Development

No branches or pull requests

2 participants