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

Add options to set the fft size and to normalize the ifft output; calculate the fft by default; and improve the documentation and the tests #3

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AngelEzquerra
Copy link

@AngelEzquerra AngelEzquerra commented Nov 6, 2023

This PR does a few small things:

  • It improves the fft function documentation
  • It adds a normalize argument to the fft and dft functions. Normalization is disabled by default but if enabled it will normalize the ifft otuput (i.e. it only applies if inverse is true)
  • It makes inverse=false the default (i.e. the fft function calculates the fft by default)
  • It adds an n argument to make it possible to choose the FFT size (it still uses the input length by default).
  • It improves the tests (which could fail if the folds were negative)

@AngelEzquerra AngelEzquerra changed the title Add option to normalize the ifft output, calculate the fft by default and improve the documentation and the tests Add options to set the fft size and to normalize the ifft output; calculate the fft by default; and improve the documentation and the tests Nov 7, 2023
@AngelEzquerra
Copy link
Author

AngelEzquerra commented Nov 7, 2023

I wonder if there is a more optimum way to implement the "Add n argument to select the FFT size" commit. That is, to only create a copy of the input when needed.

src/fftr.nim Outdated
for i in 0..<result.len:
result[i] = result[i] / complexInputLen

func fft*(input: openArray[Complex64], inverse: bool = false, normalize: bool = false, n = 0): seq[Complex64] =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding normalization, there are two kinds - 1/n and 1/sqrt(n) depending on what the numbers will be used for - see https://dsp.stackexchange.com/a/63006 and https://docs.scipy.org/doc/scipy/reference/generated/scipy.fft.fft.html#scipy.fft.fft

regarding n, this feels like outside of the domain of the fft function itself - it's a setLen away for anyone that wants to do fft this particular way (though they probably don't)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew that scipy's fft had 3 normalization options. If you think that makes sense I can support them all by passing a norm argument that would be an enum.

Regarding n, I think it would be a good idea to support it as well, following scpy's API as much as possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added norm arguments matching SCIPY's options plus a "disabled" option to completely disable the normalization. I've made the default backwards to mach SCIPY.

src/fftr.nim Outdated
input.toSeq & newSeq[Complex64](size - input.len)
elif size < input.len:
# Crop the input as much as needed
input[0..<size]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if n was pursued, this would introduce an unnecessary copy (that could be avoided with toOpenArray)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right of course. I even mentioned in my first comment on this PR :)
I will look into it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've improved the code to avoid unnecessary copies.

src/fftr.nim Outdated

let input = if size > input.len:
# Extend the input with zeros as much as needed
input.toSeq & newSeq[Complex64](size - input.len)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if n was pursued, setLen here would involve fewer copies and zeroings

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've improved the code to avoid unnecessary copies.

src/fftr.nim Outdated
# Crop the input as much as needed
input[0..<size]
else:
input.toSeq
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and another copy here - adding these copies up would significantly slow down the implementation - you should see this if you run the benchmark before and after

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I ran the benchmarks and I did not notice any significant differences, but I agree that unnecessary copies should be avoided.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've improved the code to avoid unnecessary copies.

The argument name has been chosen to emulate the numpy.fft API.
…to match the scpy normalization feature API).

The default normalization is set to `backward` to match the scpy behavior.

This commit also adds tests for the different normalization modes.
This makes the fft and ifft functions use the same argument order, as well as making this library match scpy's fft argument order.
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.

None yet

2 participants