Skip to content

Defer fft normalization to post transform stage #2185

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 15, 2018
Merged

Defer fft normalization to post transform stage #2185

merged 1 commit into from
Jun 15, 2018

Conversation

9prady9
Copy link
Member

@9prady9 9prady9 commented Jun 7, 2018 •
edited
Loading

Fixes #2050

@9prady9 9prady9 added the fix label Jun 7, 2018
@9prady9 9prady9 added this to the v3.6.1 milestone Jun 7, 2018
@9prady9 9prady9 requested review from pavanky and umar456 June 7, 2018 12:01
@9prady9
Copy link
Member Author

9prady9 commented Jun 7, 2018 •
edited
Loading

style fixes commit broke something. The tests are passing with earlier commit. Looking into it.

Edited:
Fixed!

@umar456
Copy link
Member

umar456 commented Jun 11, 2018

Can you separate the style changes from the PR? Its hard differentiate the changes in functionality from style.

@9prady9
Copy link
Member Author

9prady9 commented Jun 11, 2018 via email

@umar456
Copy link
Member

umar456 commented Jun 11, 2018

It looks like you made some style changes in your first commit.

@9prady9
Copy link
Member Author

9prady9 commented Jun 12, 2018

@umar456 Addressed feedback

umar456
umar456 requested changes Jun 14, 2018
View reviewed changes
Copy link
Member

@umar456 umar456 left a comment

Choose a reason for hiding this comment

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

Please defer style changes until we can agree on one.

src/api/c/fft.cpp Outdated
fft_inplace<T, rank, direction>(input);
if (norm_factor != 1) {
if (norm_factor != 1.0)
Copy link
Member

Choose a reason for hiding this comment

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

I would like to keep the braces around if statements.

src/api/c/fft.cpp Outdated
TYPE_ERROR(1, type);
}
case f32: {
auto f32Fn = fftR2C<float , cfloat, rank>;
Copy link
Member

Choose a reason for hiding this comment

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

This is a weird transformation. I don't like this approach.

src/api/c/fft_common.hpp Outdated
{
for (int i = 0; i < 4; i++) {
for (int i = 0; i < 4; i++)
Copy link
Member

Choose a reason for hiding this comment

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

Keep braces

src/api/c/fft.cpp Outdated
{
const dim_t pad[2] = {pad0, pad1};
return fft_r2c<2>(out, in, norm_factor, (pad0>0&&pad1>0?2:0), pad);
const bool padFlag = pad0>0&&pad1>0;
Copy link
Member

Choose a reason for hiding this comment

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

spaces between operators

src/api/c/fft.cpp Outdated
{
const dim_t pad[3] = {pad0, pad1, pad2};
return fft_r2c<3>(out, in, norm_factor, (pad0>0&&pad1>0&&pad2>0?3:0), pad);
const bool padFlag = pad0>0&&pad1>0&&pad2>0;
Copy link
Member

Choose a reason for hiding this comment

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

spaces between operators

src/backend/cpu/kernel/fftconvolve.hpp Outdated
@@ -67,8 +68,10 @@ void padArray(Param<To> out, const af::dim4 od, const af::dim4 os,
for (int d0 = 0; d0 < (int)od[0] / 2; d0++) {
const dim_t oidx = d3*os[3] + d2*os[2] + d1*os[1] + d0*2;

if (d0 < (int)id[0] && d1 < (int)id[1] && d2 < (int)id[2] && d3 < (int)id[3]) {
// Copy input elements to real elements, set imaginary elements to 0
if (d0 < (int)id[0] && d1 < (int)id[1] &&
Copy link
Member

Choose a reason for hiding this comment

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

perhaps do the conversion outside of the for loops?

src/backend/cuda/kernel/fftconvolve.hpp Outdated
@@ -117,7 +115,8 @@ __global__ void padArray(
const int t2 = to3*so3 + to2*so2 + to1*so1 + to0;

if (to0 < di0 && to1 < di1 && to2 < di2 && to3 < di3) {
// Copy input elements to real elements, set imaginary elements to 0
// Copy input elements to real elements,
Copy link
Member

Choose a reason for hiding this comment

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

This was already less than 80 characters. Why change it?

src/backend/opencl/kernel/fftconvolve.hpp Outdated
@@ -224,8 +231,8 @@ void complexMultiplyHelper(Param packed,
Param sig_tmp, filter_tmp;
calcParamSizes(sig_tmp, filter_tmp, packed, sig, filter, baseDim, kind);

int sig_packed_elem = sig_tmp.info.strides[3] * sig_tmp.info.dims[3];
int filter_packed_elem = filter_tmp.info.strides[3] * filter_tmp.info.dims[3];
int sig_packed_elem = sig_tmp.info.strides[3]*sig_tmp.info.dims[3];
Copy link
Member

Choose a reason for hiding this comment

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

spaces around operators

src/api/c/fft.cpp Outdated
case c64: output = fft<cdouble, cdouble, rank, direction>(in, norm_factor, npad, pad); break;
case f32: output = fft<float , cfloat , rank, direction>(in, norm_factor, npad, pad); break;
case f64: output = fft<double, cdouble , rank, direction>(in, norm_factor, npad, pad); break;
case c32: {
Copy link
Member

Choose a reason for hiding this comment

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

This is an odd transformation. I have an idea to fix this pattern in our code. Let's defer this change until then.

src/api/c/fftconvolve.cpp Outdated
fftConv<Ti, Tr, Tc, F0 , F1, baseDim>(signal, filter, expand, convBT)

#define FALLBACK(T) \
fftConvFallback<T , T , T , baseDim>(signal, filter, expand)
Copy link
Member

Choose a reason for hiding this comment

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

You can do the same thing using some combination of type traits instead of creating macros. You don't need macros here.

@9prady9
Copy link
Member Author

9prady9 commented Jun 15, 2018

Rolled back style changes for the moment.

@9prady9 9prady9 merged commit 04d298d into arrayfire:master Jun 15, 2018
@9prady9 9prady9 deleted the defer_fft_norm branch June 15, 2018 10:23
@net147
Copy link

net147 commented Jun 15, 2018

@9prady9 Does the documentation need to be updated? The documentation has norm_factor as scaling the input before the transformation is applied.

@9prady9
Copy link
Member Author

9prady9 commented Jun 15, 2018

@net147 This doesn't require docs udpate because we didn't normalization in this commit. We merely changed the stage at which normalization is carried out. If a normalization factor other than 1.0 is provided, we use that. The API already has the normalization factor as parameter.

@net147
Copy link

net147 commented Jun 15, 2018 •
edited
Loading

@9prady9 Yes the API has normalization factor as a parameter, but the documentation says that the factor is applied before the transformation but this commit changes it to be applied after the transformation:
norm_factor "is the normalization factor with which the input is scaled before the transformation is applied"

@9prady9
Copy link
Member Author

9prady9 commented Jun 15, 2018

Sorry, I didn't notice that it was specific about when it's applied. I will change it, thanks for reporting!

@9prady9
Copy link
Member Author

9prady9 commented Jun 15, 2018

@net147 I have addressed it in #2192 thanks once again

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

@umar456 umar456 umar456 approved these changes

@pavanky pavanky Awaiting requested review from pavanky

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
v3.6.1
Development

Successfully merging this pull request may close these issues.
3 participants
@9prady9 @umar456 @net147