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

Combine dct and qct interfaces #35

Merged
merged 5 commits into from
Mar 8, 2023
Merged

Combine dct and qct interfaces #35

merged 5 commits into from
Mar 8, 2023

Conversation

cval26
Copy link
Contributor

@cval26 cval26 commented Feb 28, 2023

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) and qct(x,n) families of procedures under one interface dct(x,n,type), where type=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 set type=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.

@cval26 cval26 marked this pull request as draft March 1, 2023 01:30
@cval26 cval26 marked this pull request as ready for review March 2, 2023 17:49
@cval26
Copy link
Contributor Author

cval26 commented Mar 2, 2023

This PR is ready for review, @zoziha. The main changes are:

  • combined dct(x,n) and qct(x,n) under one interface dct(x,n,type);
  • combined all previous dct/qct tests and added a few more;
  • updated and improved the DCT-related documentation, so that it's clear which DCT type each procedure corresponds to.

The diff for the documentation file fftpack.md is rather large, because I had to change the heading levels throughout the document to make the headings consistent. Most of the meaningful changes are in the DCT section. The file is more easily viewed rendered in an editor.

Copy link
Contributor

@zoziha zoziha left a 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.

src/fftpack_dct.f90 Outdated Show resolved Hide resolved
doc/specs/fftpack.md Show resolved Hide resolved
@certik certik requested a review from jacobwilliams March 3, 2023 04:38
@certik
Copy link
Member

certik commented Mar 3, 2023

@jacobwilliams can you please review this?

@cval26
Copy link
Contributor Author

cval26 commented Mar 5, 2023

@jacobwilliams any updates on this?

@zoziha
Copy link
Contributor

zoziha commented Mar 7, 2023

According to previous experience, the number of reviewers of fftpack is often insufficient, which is a practical problem. Since the dct interface is a high-level interface, this PR does not change the low-level interface and maintains the unity with the scipy.fftpack.dct interface, and the code passes the test.

I also compared the result of dct(fftpack) with scipy, DCT-type-1 / DCT-type-3 is consistent with the behavior of scipy, for DCT-type-2, the result of fftpack is exactly twice as much as scipy's at present. The specific reason is unknown, but it is not caused by this PR.

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

@cval26
Copy link
Contributor Author

cval26 commented Mar 7, 2023

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.

@zoziha zoziha merged commit b745bb8 into fortran-lang:main Mar 8, 2023
@cval26 cval26 deleted the dct-interface branch March 19, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve interface of discrete cosine transforms
3 participants