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

ENH: Normalise the inverse DCT/DST in scipy.fft #10470

Merged
merged 8 commits into from Jul 17, 2019

Conversation

@peterbell10
Copy link
Member

commented Jul 16, 2019

Closes #3677

As described in the issue, this adds the correct normalization factor to the inverse dct/dst such that the identity idct(dct(x)) == x is true for all normalization modes. This is consistent with the other FFT functions in scipy.fft

I will fix the merge conflicts with #10383 after one is merged. ✔️

@peterbell10 peterbell10 force-pushed the peterbell10:idct-norm branch 2 times, most recently from 6b4f16d to de56ca9 Jul 16, 2019

@grlee77
Copy link
Contributor

left a comment

This looks correct to me. I just had a couple of minor comments.


y2 = np.pad(y, pad, mode='edge')
z2 = backward(y2, type, n, axis, norm)
assert_allclose(z2, x)

This comment has been minimized.

Copy link
@grlee77

grlee77 Jul 16, 2019

Contributor

I may have missed it elsewhere, but are there any tests that verify that the energy a signal before and after the transform is the same when norm='ortho'?

i.e. assert_allclose(np.linalg.norm(x), np.linalg.norm(y)) should pass when norm='ortho'

This comment has been minimized.

Copy link
@peterbell10

peterbell10 Jul 16, 2019

Author Member

Here I am focusing on testing the differences from scipy.fftpack and letting test_fftpack_equivalence cover the rest. Does that make sense?

This comment has been minimized.

Copy link
@grlee77

grlee77 Jul 16, 2019

Contributor

Yeah, that made sense. I was just wondering whether this property was ever tested in FFTPack either? That is not strictly an issue for this PR, though, so I approved it as-is.

@@ -158,7 +158,7 @@ def test_nonlinear_constraint():
def test_concatenation():
rng = np.random.RandomState(0)
n = 4
x0 = np.random.rand(n)
x0 = rng.rand(n)

This comment has been minimized.

Copy link
@peterbell10

peterbell10 Jul 16, 2019

Author Member

If this seems out of place, it is needed to fix the test failure from #10473.

@peterbell10 peterbell10 force-pushed the peterbell10:idct-norm branch from 6b18758 to 3068fb3 Jul 17, 2019

@larsoner
Copy link
Member

left a comment

LGTM and already have a +1 for merge from @grlee77, so I'll merge. Thanks @peterbell10 !

Feel free to take care of the normalization check in another PR at some point if you want, but I wouldn't consider it a high priority.

@larsoner larsoner merged commit 35f86bc into scipy:master Jul 17, 2019

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 #20190717.8 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

@peterbell10 peterbell10 deleted the peterbell10:idct-norm branch Jul 17, 2019

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

I've added to the release notes:

the inverse real to real transforms (idct and idst) are normalized for norm=None in the same way as ifft. This means the identity idct(dct(x)) == x is now true for all norm modes.

@pv pv added this to the 1.4.0 milestone Jul 18, 2019

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