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

TST: Update scipy.fft real transform tests #10669

Merged
merged 7 commits into from Aug 24, 2019

Conversation

@peterbell10
Copy link
Member

peterbell10 commented Aug 15, 2019

What does this implement/fix?

This is a general cleanup of the tests for dct and dst in scipy.fft. Specific improvements include:

  • using pytest parametrized tests instead of seperate classes
  • grouping tolerances together into a single dict
  • adding a parametrization for np.longfloat to all transforms
  • tightening test tolerances

Additional information

I was also planning to address @larsoner's comment (#10493 (comment)) about changing the FFTWDATA globals into a fixture that implicitly parametrizes the test with the different sizes but ran into issues. pytest seems to require that test parametrization be known during collection which means that the data file needs to have already been loaded.

So, I think that a fixture can only be used if the tests go back to explicitly iterating over the different sizes. i.e. instead of

def test(fftwdata_size):
	# test one size

it would have to be

def test(fftwdata_sizes):
	for size in fftwdata_sizes:
        # test one size

Which of the two would be preferred and is there another way around the collection issue?

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Aug 15, 2019

@peterbell10 if we encode the number of sizes n_sizes to be tested in the file (at a slight DRY cost) that matches the number in the actual file then we can use this:

https://pytest.readthedocs.io/en/latest/example/parametrize.html#deferring-the-setup-of-parametrized-resources

You could parametrize to run np.arange(n_sizes) and deferred-load the actual size.

@peterbell10

This comment has been minimized.

Copy link
Member Author

peterbell10 commented Aug 15, 2019

Can the conftest.py file be local to just the scipy.fft tests?

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Aug 15, 2019

I think you can nest conftest.py files, so you could add one in scipy/fft.

@peterbell10 peterbell10 force-pushed the peterbell10:real-transform-tests branch 2 times, most recently from d28b370 to 4a70a14 Aug 15, 2019
@peterbell10

This comment has been minimized.

Copy link
Member Author

peterbell10 commented Aug 15, 2019

Tests are failing on 32-bit linux because the long double reference data isn't binary compatible. Is there an alternative file format I could use or should I just skip the test?

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Aug 15, 2019

You could write it to .mat with scipy.io. It would be good to make the tests work on 32-bit somehow.

@peterbell10 peterbell10 force-pushed the peterbell10:real-transform-tests branch 2 times, most recently from d8fad7d to 6d08b7e Aug 15, 2019
@peterbell10

This comment has been minimized.

Copy link
Member Author

peterbell10 commented Aug 16, 2019

I just checked and savemat is actually casting to np.float64 before writing to the file.

@peterbell10 peterbell10 force-pushed the peterbell10:real-transform-tests branch 2 times, most recently from 047faab to cce4ae0 Aug 16, 2019
Copy link
Member

larsoner left a comment

I just checked and savemat is actually casting to np.float64 before writing to the file.

Bummer. I'm fine with skipping the longdouble ones on 32-bit then.

+1 for merge from me. @grlee77 do you want to have a look?

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Aug 16, 2019

(Travis failure appears to be unrelated, see #10676)

@peterbell10 peterbell10 force-pushed the peterbell10:real-transform-tests branch from cce4ae0 to 349e3be Aug 19, 2019
@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Aug 24, 2019

I'll go ahead and merge this one. It's mostly a refactor/test modernization so shouldn't be too controversial.

@larsoner larsoner merged commit fe622e7 into scipy:master Aug 24, 2019
9 checks passed
9 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scipy.scipy Build #20190819.15 succeeded
Details
scipy.scipy (Linux_Python_36_32bit_full) Linux_Python_36_32bit_full succeeded
Details
scipy.scipy (Windows Python35-64bit-full) Windows Python35-64bit-full succeeded
Details
scipy.scipy (Windows Python36-32bit-full) Windows Python36-32bit-full succeeded
Details
scipy.scipy (Windows Python36-64bit-full) Windows Python36-64bit-full succeeded
Details
scipy.scipy (Windows Python37-64bit-full) Windows Python37-64bit-full succeeded
Details
@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Aug 24, 2019

Thanks @peterbell10

@peterbell10 peterbell10 deleted the peterbell10:real-transform-tests branch Aug 24, 2019
@tylerjereddy tylerjereddy added this to the 1.4.0 milestone Aug 25, 2019
@@ -57,3 +59,18 @@ def gen_data(dt):
for sz in SZ:
d['dst_%d_%d' % (type-4, sz)] = data[type][sz]
np.savez(filename, **d)

# generate long double precision data
data = gen_data(np.float128)

This comment has been minimized.

Copy link
@tylerjereddy

tylerjereddy Aug 26, 2019

Contributor

Portability to ppc64le native hardware? see #9684 after rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.