This repository has been archived by the owner. It is now read-only.

Change of FFT implementation to Jtransforms #508

Merged
merged 5 commits into from Feb 16, 2018

Conversation

Projects
None yet
6 participants
@DrChainsaw
Copy link
Contributor

DrChainsaw commented Feb 15, 2018

What changes were proposed in this pull request?

Changed FFT implementation to JTransforms as discussed in #505.
I might have gone a bit overboard with the backwards compatibility this time around: Previous implementation returned an array which was four times smaller than the input when input was complex.
This is what you get with the method without the boolean argument while the one with the boolean arg returns an array of half the size as the input for both complex and real amplitudes.

How was this patch tested?

Unit tests

Please review
https://github.com/deeplearning4j/deeplearning4j/blob/master/CONTRIBUTING.md before opening a pull request.

DrChainsaw added some commits Feb 7, 2018

Two missing files for previous commit
Corrections for issue 502:
#502
Updates according to review comments.
Reverted to legacy method signature for complex interlaced amplitudes and added flag to select assumptions about amplitudes format for new method
@agibsonccc

This comment has been minimized.

Copy link
Member

agibsonccc commented Feb 15, 2018

@DrChainsaw this is great, I'll let @AlexDBlack comment on this but I don't have any objections so far.

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Feb 16, 2018

I scanned this quickly, looks fine to me.
I'll defer to @saudet for the final review/merge.

@saudet saudet merged commit 6dcdccb into deeplearning4j:master Feb 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@treo

This comment has been minimized.

Copy link
Member

treo commented Feb 26, 2018

To me it looks like there should be a slight change: As this only pertains to datavec-data-audio, the jtransforms dependency should be placed in the corresponding pom.xml. Putting it right into the main pom.xml makes all datavec packages depend on it.

@Ngosti2000

This comment has been minimized.

Copy link

Ngosti2000 commented Feb 26, 2018

@treo ..yeap makes sense to separate as my android build is failing..have raised the issue on it as well. #4718

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.