Migrate Python tests to pytest #265

lgarrison · 2021-11-19T20:52:38Z

From #258, we had an OpenMP error that only showed up in the Python tests. Therefore we decided to be a bit more organized about the Python tests, and migrate to pytest. The code is pretty compact, and tests all ISA for all main API functions.

These tests all use the same data and configuration as the C tests, and hence we have the external, correct results to compare against.

Once this gets into master, we can merge/rebase master back into linearbinning. I have a corresponding test_lin.py that we can put into linearbinning once that happens.

pep8speaks · 2021-11-19T20:52:42Z

Hello @lgarrison! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file Corrfunc/__init__.py:

Line 20:80: E501 line too long (103 > 79 characters)

In the file Corrfunc/io.py:

Line 43:1: W293 blank line contains whitespace
Line 136:50: E231 missing whitespace after ','
Line 136:54: E231 missing whitespace after ','

In the file Corrfunc/tests/common.py:

Line 9:1: E302 expected 2 blank lines, found 1
Line 11:1: W293 blank line contains whitespace
Line 13:21: E128 continuation line under-indented for visual indent
Line 15:1: W293 blank line contains whitespace
Line 22:17: E128 continuation line under-indented for visual indent
Line 23:7: E231 missing whitespace after ','
Line 23:11: E231 missing whitespace after ','
Line 23:14: E231 missing whitespace after ','
Line 24:1: W293 blank line contains whitespace
Line 31:17: E128 continuation line under-indented for visual indent
Line 32:7: E231 missing whitespace after ','
Line 32:11: E231 missing whitespace after ','
Line 32:14: E231 missing whitespace after ','
Line 33:1: W293 blank line contains whitespace
Line 40:1: W293 blank line contains whitespace
Line 45:1: W293 blank line contains whitespace
Line 50:80: E501 line too long (91 > 79 characters)
Line 52:1: W293 blank line contains whitespace
Line 56:22: E231 missing whitespace after ','
Line 57:1: W293 blank line contains whitespace
Line 59:26: E231 missing whitespace after ','
Line 59:34: E231 missing whitespace after ','
Line 59:40: E231 missing whitespace after ','
Line 62:20: E231 missing whitespace after ','
Line 63:26: E231 missing whitespace after ','
Line 64:1: W293 blank line contains whitespace
Line 76:1: W293 blank line contains whitespace
Line 77:22: E231 missing whitespace after ','
Line 77:34: E231 missing whitespace after ','
Line 78:23: E231 missing whitespace after ','
Line 78:34: E231 missing whitespace after ','
Line 79:16: E711 comparison to None should be 'if cond is not None:'
Line 83:1: W293 blank line contains whitespace
Line 86:25: E124 closing bracket does not match visual indentation
Line 87:1: W293 blank line contains whitespace
Line 92:1: E302 expected 2 blank lines, found 1
Line 96:1: W293 blank line contains whitespace
Line 98:51: E231 missing whitespace after ','
Line 100:25: E124 closing bracket does not match visual indentation

In the file Corrfunc/tests/test_mocks.py:

Line 12:1: W293 blank line contains whitespace
Line 17:1: W293 blank line contains whitespace
Line 20:22: E127 continuation line over-indented for visual indent
Line 23:1: W293 blank line contains whitespace
Line 24:7: E231 missing whitespace after ','
Line 24:11: E231 missing whitespace after ','
Line 24:14: E231 missing whitespace after ','
Line 26:31: E128 continuation line under-indented for visual indent
Line 27:31: E128 continuation line under-indented for visual indent
Line 28:31: E128 continuation line under-indented for visual indent
Line 29:31: E128 continuation line under-indented for visual indent
Line 30:31: E128 continuation line under-indented for visual indent
Line 31:1: W293 blank line contains whitespace
Line 33:21: E128 continuation line under-indented for visual indent
Line 34:80: E501 line too long (98 > 79 characters)
Line 36:1: W293 blank line contains whitespace
Line 40:1: W293 blank line contains whitespace
Line 42:22: E127 continuation line over-indented for visual indent
Line 47:1: W293 blank line contains whitespace
Line 48:7: E231 missing whitespace after ','
Line 48:11: E231 missing whitespace after ','
Line 48:14: E231 missing whitespace after ','
Line 50:31: E128 continuation line under-indented for visual indent
Line 51:31: E128 continuation line under-indented for visual indent
Line 52:31: E128 continuation line under-indented for visual indent
Line 53:31: E128 continuation line under-indented for visual indent
Line 54:31: E128 continuation line under-indented for visual indent
Line 55:1: W293 blank line contains whitespace
Line 57:21: E128 continuation line under-indented for visual indent
Line 58:80: E501 line too long (96 > 79 characters)
Line 59:1: W293 blank line contains whitespace
Line 60:1: W293 blank line contains whitespace
Line 64:1: W293 blank line contains whitespace
Line 67:22: E127 continuation line over-indented for visual indent
Line 68:1: W293 blank line contains whitespace
Line 69:7: E231 missing whitespace after ','
Line 69:11: E231 missing whitespace after ','
Line 69:14: E231 missing whitespace after ','
Line 71:31: E128 continuation line under-indented for visual indent
Line 72:31: E128 continuation line under-indented for visual indent
Line 73:31: E128 continuation line under-indented for visual indent
Line 74:31: E128 continuation line under-indented for visual indent
Line 75:31: E128 continuation line under-indented for visual indent
Line 76:1: W293 blank line contains whitespace
Line 78:21: E128 continuation line under-indented for visual indent
Line 79:80: E501 line too long (102 > 79 characters)
Line 85:1: W293 blank line contains whitespace
Line 98:1: W293 blank line contains whitespace
Line 100:22: E127 continuation line over-indented for visual indent
Line 101:1: W293 blank line contains whitespace
Line 102:7: E231 missing whitespace after ','
Line 102:11: E231 missing whitespace after ','
Line 102:14: E231 missing whitespace after ','
Line 104:37: E127 continuation line over-indented for visual indent
Line 105:37: E127 continuation line over-indented for visual indent
Line 106:37: E127 continuation line over-indented for visual indent
Line 106:62: W291 trailing whitespace
Line 107:37: E127 continuation line over-indented for visual indent
Line 108:1: W293 blank line contains whitespace
Line 110:21: E128 continuation line under-indented for visual indent
Line 112:1: W293 blank line contains whitespace
Line 112:1: W391 blank line at end of file

In the file Corrfunc/tests/test_theory.py:

Line 17:1: W293 blank line contains whitespace
Line 20:22: E127 continuation line over-indented for visual indent
Line 23:1: W293 blank line contains whitespace
Line 24:6: E231 missing whitespace after ','
Line 24:8: E231 missing whitespace after ','
Line 24:10: E231 missing whitespace after ','
Line 26:29: E127 continuation line over-indented for visual indent
Line 27:29: E127 continuation line over-indented for visual indent
Line 28:29: E127 continuation line over-indented for visual indent
Line 29:29: E127 continuation line over-indented for visual indent
Line 30:1: W293 blank line contains whitespace
Line 32:21: E128 continuation line under-indented for visual indent
Line 35:1: W293 blank line contains whitespace
Line 40:1: W293 blank line contains whitespace
Line 44:22: E127 continuation line over-indented for visual indent
Line 47:1: W293 blank line contains whitespace
Line 48:6: E231 missing whitespace after ','
Line 48:8: E231 missing whitespace after ','
Line 48:10: E231 missing whitespace after ','
Line 54:1: W293 blank line contains whitespace
Line 56:21: E128 continuation line under-indented for visual indent
Line 57:80: E501 line too long (92 > 79 characters)
Line 59:1: W293 blank line contains whitespace
Line 63:1: W293 blank line contains whitespace
Line 66:22: E127 continuation line over-indented for visual indent
Line 71:1: W293 blank line contains whitespace
Line 72:6: E231 missing whitespace after ','
Line 72:8: E231 missing whitespace after ','
Line 72:10: E231 missing whitespace after ','
Line 74:29: E127 continuation line over-indented for visual indent
Line 75:29: E127 continuation line over-indented for visual indent
Line 76:29: E127 continuation line over-indented for visual indent
Line 77:29: E127 continuation line over-indented for visual indent
Line 78:29: E127 continuation line over-indented for visual indent
Line 79:29: E127 continuation line over-indented for visual indent
Line 80:1: W293 blank line contains whitespace
Line 82:21: E128 continuation line under-indented for visual indent
Line 83:80: E501 line too long (90 > 79 characters)
Line 84:1: W293 blank line contains whitespace
Line 85:1: W293 blank line contains whitespace
Line 86:80: E501 line too long (94 > 79 characters)
Line 89:1: W293 blank line contains whitespace
Line 93:22: E127 continuation line over-indented for visual indent
Line 94:1: W293 blank line contains whitespace
Line 95:6: E231 missing whitespace after ','
Line 95:8: E231 missing whitespace after ','
Line 95:10: E231 missing whitespace after ','
Line 97:29: E127 continuation line over-indented for visual indent
Line 98:29: E127 continuation line over-indented for visual indent
Line 99:29: E127 continuation line over-indented for visual indent
Line 100:29: E127 continuation line over-indented for visual indent
Line 101:1: W293 blank line contains whitespace
Line 103:21: E128 continuation line under-indented for visual indent
Line 104:80: E501 line too long (82 > 79 characters)
Line 106:1: W293 blank line contains whitespace
Line 111:1: W293 blank line contains whitespace
Line 114:22: E127 continuation line over-indented for visual indent
Line 115:1: W293 blank line contains whitespace
Line 116:6: E231 missing whitespace after ','
Line 116:8: E231 missing whitespace after ','
Line 116:10: E231 missing whitespace after ','
Line 118:29: E127 continuation line over-indented for visual indent
Line 119:29: E127 continuation line over-indented for visual indent
Line 120:29: E127 continuation line over-indented for visual indent
Line 121:29: E127 continuation line over-indented for visual indent
Line 122:1: W293 blank line contains whitespace
Line 124:21: E128 continuation line under-indented for visual indent
Line 125:80: E501 line too long (104 > 79 characters)
Line 126:1: W293 blank line contains whitespace
Line 131:1: W293 blank line contains whitespace
Line 139:1: W293 blank line contains whitespace
Line 140:6: E231 missing whitespace after ','
Line 140:8: E231 missing whitespace after ','
Line 140:10: E231 missing whitespace after ','
Line 141:16: E221 multiple spaces before operator
Line 142:27: E127 continuation line over-indented for visual indent
Line 143:27: E127 continuation line over-indented for visual indent
Line 144:5: E265 block comment should start with '# '
Line 144:80: E501 line too long (106 > 79 characters)
Line 146:21: E128 continuation line under-indented for visual indent

Comment last updated at 2021-12-02 15:50:15 UTC

lgarrison · 2021-11-19T21:17:25Z

@manodeep the Python 2.7 tests have trouble with importing this module structure. And looking ahead to test_lin.py, Python 2.7 doesn't like some of the syntax there. Okay if I drop Python 2.7 from the Python test stage of the CI? Or even remove it from the CI matrix completely, unless we want to keep testing the build of the C extensions against Python 2.7.

manodeep · 2021-11-23T00:19:49Z

Only just got a chance to take a look - not sure I follow what's going on with the test failure - could it just be a missing __init__.py file?

We should try to support python 2.7 for at least Corrfunc v2.5, and then drop the py2.7 support from Corrfunc v2.6. Would it work to add a deprecation warning in v2.5?

manodeep · 2021-11-23T04:57:21Z

We should also test for some values of nthreads - say [1, 2, 3, 4] - perhaps with isa=fastest. That would (hopefully) detect any OpenMP threading issues

lgarrison · 2021-11-24T00:31:33Z

This version tests all ISA for the max number of threads (inferred from cores/affinity), and thread counts from 1 to min(4,maxthreads) for the fastest ISA. Unfortunately, that's a lot of combos, so CI time has increased quite a bit; the Python tests alone take over 10 minutes. I'm open to suggestions on smart ways to reduce the number of tests...

In addition, this does add one test case with nthreads = ncores + 1, just to check for correctness in that corner case.

manodeep · 2021-11-24T04:52:34Z

Looking at the CI failure, looks like os.cpu_count() does not exist in python2.7. That AttributeError should be fixable by a try but setting the actual number of threads correctly might be more difficult. Saw this solution on SO - which we can rip off and keep only the POSIX relevant bits. What do you think?

lgarrison · 2021-11-24T22:03:27Z

Working now, I think!

manodeep · 2021-11-29T02:14:14Z

Still haven't had time to review - will look in the next 24-48 hours at the latest!

manodeep

I did a quick skim of the code - looks pretty good!

Do we need to figure out a way to reduce the runtime?

Corrfunc/tests/common.py

…ty for npairs; other code review.

lgarrison · 2021-12-01T17:38:45Z

Here's a version that refactors the reference check to use column names where possible! And, most importantly, the npairs check now uses an integer equality test rather than some tolerance.

On the runtime, I'm considering the fact that the ISA sweep already tests the multi-threaded correctness. So what about reducing the thread sweep to two values: 1 and ncore+1. This tests single-threaded and "oversubscribed" correctness. Because the ISA sweep uses ncores threads, ncores+1 also ensures we test both an even and odd number of threads. I think that's pretty good coverage: single-threaded, even-number multithreaded, odd-number multithreaded, and oversubscribed multithreaded.

Corrfunc/io.py

Corrfunc/tests/common.py

Corrfunc/tests/test_mocks.py

manodeep · 2021-12-02T02:44:19Z

Corrfunc/tests/test_theory.py

+                            weights1=w, weight_type='pair_product',
+                            periodic=periodic, boxsize=boxsize,
+                            output_ravg=True, verbose=True,
+                            isa=isa)


Whitespace issues here - the second line of parameters should probably align with the opening bracket on the first line (or whatever PEP recommends). Similar alignment are there elsewhere - will you please fix if possible.

(We really need to get a linter into our CI/pre-commit)

Yeah, there's a lot of whitespace issues! I clearly need to upgrade to an editor that will flag PEP8 issues on-the-fly. But I think the codebase is littered with whitespace problems, and it's probably more efficient if we do a big PEP8 linting pass on the entire codebase, rather than playing whack-a-mole each time we open a PR. So I may leave this for now, if that's okay.

manodeep · 2021-12-02T03:00:14Z

Everything looks good to me - I made some minor comments, mostly related to whitespace and (likely) pytests wizardry

lgarrison · 2021-12-02T15:05:13Z

Should I go ahead and reduce the thread test cases as proposed above?

manodeep · 2021-12-02T17:53:30Z

Should I go ahead and reduce the thread test cases as proposed above?

Ahh yes - that should be a good enough nthreads coverage. And as long as the single thread agrees with a few different multiple-thread, we are probably okay (haven't tested that though)

lgarrison · 2021-12-02T18:05:38Z

Great -- single- & multi-threaded both agree with the known answer, so this is a pretty good test I think.

I'll merge this and then create a test_lin.py in linearbinning, and once that passes we'll be ready to merge linearbinning!

* Fix very minor thread race * Add stripped-down test_periodic_lin * Made the test messages/code more user-friendly * Added more comprehensive tests for linear bins * Had commented out the required INTEGRATION_TESTS * Also need to invoke the CI testing * More info on failed bins, and continue to test all bins * Moved declarations around * Fix AVX2/AVX/SSE linear binning bug in theory * add comparison to reference in call_correlation_functions python calls * Migrate Python tests to pytest (#265) * Migrate Python tests to pytest * Backport fastfood weights * Fix CPU count for OSX * Add deprecation warning for Python < 3 * Try to fix Python 2.7 * Test all ISA with max threads, and all nthreads with fastest ISA * Try again to fix Python 2.7 * Refactor reference check to use column names and exact integer equality for npairs; other code review. * Fixed typo in changelog * Add AVX2 to wp test. Change thread sweep config. Other code review. * Fix tests Co-authored-by: Manodeep Sinha <manodeep@gmail.com> * Add test_lin.py to pytests * Fix bin file precision in linear C tests * Fix nthreads * Don't test linear binning on Python 2.7 * Try to support old Numpy * fixing issue in theta AVX linear binning Co-authored-by: Arnaud De-Mattia <adematti@spppcx147.extra.cea.fr> Co-authored-by: Lehman Garrison <lgarrison@flatironinstitute.org> Co-authored-by: Manodeep Sinha <manodeep@gmail.com>

Migrate Python tests to pytest

e3e4753

lgarrison requested a review from manodeep November 19, 2021 20:52

Backport fastfood weights

96d06e5

lgarrison added this to the v2.5.0 milestone Nov 19, 2021

Fix CPU count for OSX

887de69

lgarrison added 3 commits November 23, 2021 15:08

Add deprecation warning for Python < 3

49e4b4f

Try to fix Python 2.7

bdb4fa7

Test all ISA with max threads, and all nthreads with fastest ISA

04bf6fd

Try again to fix Python 2.7

1d12a05

lgarrison force-pushed the pytest branch from 215f34c to 1d12a05 Compare November 24, 2021 15:00

manodeep reviewed Dec 1, 2021

View reviewed changes

Refactor reference check to use column names and exact integer equali…

1f66a31

…ty for npairs; other code review.

Fixed typo in changelog

5e9d15c

manodeep reviewed Dec 2, 2021

View reviewed changes

Corrfunc/io.py Outdated Show resolved Hide resolved

manodeep reviewed Dec 2, 2021

View reviewed changes

Corrfunc/tests/common.py Show resolved Hide resolved

manodeep reviewed Dec 2, 2021

View reviewed changes

Corrfunc/tests/test_mocks.py Show resolved Hide resolved

manodeep reviewed Dec 2, 2021

View reviewed changes

manodeep approved these changes Dec 2, 2021

View reviewed changes

Add AVX2 to wp test. Change thread sweep config. Other code review.

a343f05

lgarrison added 2 commits December 2, 2021 10:27

Merge branch 'pytest' of github.com:manodeep/Corrfunc into pytest

f6caf51

Fix tests

e9e87f3

lgarrison merged commit 102e6ec into master Dec 2, 2021

lgarrison deleted the pytest branch December 2, 2021 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Python tests to pytest #265

Migrate Python tests to pytest #265

lgarrison commented Nov 19, 2021

pep8speaks commented Nov 19, 2021 •

edited

lgarrison commented Nov 19, 2021 •

edited

manodeep commented Nov 23, 2021

manodeep commented Nov 23, 2021

lgarrison commented Nov 24, 2021

manodeep commented Nov 24, 2021

lgarrison commented Nov 24, 2021

manodeep commented Nov 29, 2021

manodeep left a comment

lgarrison commented Dec 1, 2021

manodeep Dec 2, 2021

lgarrison Dec 2, 2021

manodeep commented Dec 2, 2021

lgarrison commented Dec 2, 2021 •

edited

manodeep commented Dec 2, 2021

lgarrison commented Dec 2, 2021

Migrate Python tests to pytest #265

Migrate Python tests to pytest #265

Conversation

lgarrison commented Nov 19, 2021

pep8speaks commented Nov 19, 2021 • edited

Comment last updated at 2021-12-02 15:50:15 UTC

lgarrison commented Nov 19, 2021 • edited

manodeep commented Nov 23, 2021

manodeep commented Nov 23, 2021

lgarrison commented Nov 24, 2021

manodeep commented Nov 24, 2021

lgarrison commented Nov 24, 2021

manodeep commented Nov 29, 2021

manodeep left a comment

Choose a reason for hiding this comment

lgarrison commented Dec 1, 2021

manodeep Dec 2, 2021

Choose a reason for hiding this comment

lgarrison Dec 2, 2021

Choose a reason for hiding this comment

manodeep commented Dec 2, 2021

lgarrison commented Dec 2, 2021 • edited

manodeep commented Dec 2, 2021

lgarrison commented Dec 2, 2021

pep8speaks commented Nov 19, 2021 •

edited

lgarrison commented Nov 19, 2021 •

edited

lgarrison commented Dec 2, 2021 •

edited