-
Notifications
You must be signed in to change notification settings - Fork 19
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
Combine dct and qct interfaces #35
Conversation
This PR is ready for review, @zoziha. The main changes are:
The diff for the documentation file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cval26 . This looks good.
I leave you with a few suggestions, which may not be necessary.
@jacobwilliams can you please review this? |
@jacobwilliams any updates on this? |
According to previous experience, the number of reviewers of I also compared the result of # python
> from numpy import *
> from scipy.fftpack import *
> dct(array([9,-9,3,0]), 2)
array([ 6. , 7.44542921, 21.21320344, 29.06141056])
# Fortran
> print *, dct([real(8)::9,-9,3,0],type=2)
12.0, 14.890858416882008, 42.426406871192853, 58.122821125684993 I'm going to merge it today. If we find any problems, we will be able to make subsequent corrections. |
Thank you for taking care of this, @zoziha. The reason for the factor-of-two difference is that there are several definitions for the DCTs in use, which primarily differ in the selection of the constants. FFTPACK uses a different constant in the definition of DTC-2, compared to SciPy and also FFTW. I think that the choice of FFTW and SciPy is the more natural one and I can argue why, but changing this requires a change in the low-level interface, which should be a different PR subject to further discussion. |
Closes #34 item 1; also related to #9, #22. See #34 for the motivation behind this PR.
This PR combines the interfaces of
dct(x,n)
andqct(x,n)
families of procedures under one interfacedct(x,n,type)
, wheretype=1,2,3
is the type of DCT to be performed. This brings the modern interface of DCT in line with current DCT terminology (see the Wikipedia article for more details).For the new
dct(x,n,type)
interface, I settype=2
as the default value. I have no personal preference over the default value, but it seems that DCT-2 is the most popular DCT because of its widespread use in data compression. This is also the default DCT used in SciPy and Matlab, so I believe it makes sense that we use it too.All tests run successfully in my local build.