Skip to content

Feature: Implementation of fourier transform #2540

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

Merged
merged 24 commits into from
May 24, 2022
Merged

Feature: Implementation of fourier transform #2540

merged 24 commits into from
May 24, 2022

Conversation

HanchaiN
Copy link
Contributor

Fix #46

Fix #46

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Draft Implementations
Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

Nice, this looks good @HanchaiN , thanks a lot!

I made a few inline feedbacks, can you have a look at those?

@josdejong
Copy link
Owner

Thanks for the updates @HanchaiN , looks good 👍

Two last remarks:

  1. Can you remove the usages of math.evaluate from the unit tests? To keep the tests more isolated from the rest of the application (evaluate is pulling in "everything" basically).
  2. Can you add type definitions for the two new functions fft and ifft in ./types/index.d.ts, and write tests for it in ./types/index.ts? Then, the functions can be used in TypeScript too.

@josdejong
Copy link
Owner

Thanks for the updates @HanchaiN. I see the two new TypeScript tests fail because the output is a matrix with a couple of complex values of which the imaginary part is 0 or -0, but the tests fails because the expected outcome are all real numbers. What would be the right outcome?

HanchaiN added 6 commits May 21, 2022 18:25
Edit example in docs (`math.fft` returns complex matrix).
Edit example in docs (`math.ffti` returns complex matrix).
Edit docs examples, representation of complex number from `a+bi` to `{re:a, im:b}`
Edit docs examples, representation of complex number from `a+bi` to `{re:a, im:b}`
Edit test.
Add test for `math.ifft`
`math.fft` returns complex matrix.
@HanchaiN
Copy link
Contributor Author

Apologies for the long break before the update. The test failed when compare {re:1, im:0} and {re:1, im:-0} using assert.deepStrictEqual. However, the function in assert that only approximately compares the value cannot be found. Therefore, the code will be left in this state for a while.

@josdejong
Copy link
Owner

Ahh, good idea, you could try use approx.deepEqual, I think import * as approx from "../tools/approx.js" should work

HanchaiN added 2 commits May 23, 2022 23:44
Use `approx.deepEqual` instead off `assert.deepStrictEqual`.
Format code
@HanchaiN
Copy link
Contributor Author

It does not work either. But assert.ok(math.deepEqual(...)) might works. Let's try that and see.

HanchaiN added 2 commits May 23, 2022 23:55
Use `assert.ok(math.deepEqual(...))` instead of `approx.deepEqual`.
@josdejong
Copy link
Owner

Thanks for the updates @HanchaiN , I see you found a working solution with assert.ok(math.deepEqual(...)) 👍 . Still, it's odd that the outcome is not exactly the same on differing machines (0 or -0), but it's a bit out of scope of this PR I think to figure that out.

@josdejong josdejong merged commit ca3229f into josdejong:develop May 24, 2022
@HanchaiN HanchaiN deleted the feat--fast-fourier-transform branch May 24, 2022 07:45
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.

FFT functions
2 participants