npadmana / DistributedFFT

6 stars 2 forks source link

Make some elegance/style improvements to the elegance branch #28

Closed ronawho closed 5 years ago

ronawho commented 5 years ago

Some of these might be style preference, and some of them are comments that helped me understand the batching code better. I have no problem sticking with your preferred style if there is something you don't like here. I did not see any noticeable performance difference for the elegant=false code before and after these changes.

Here are the changes I made roughly in the order they appear:

ronawho commented 5 years ago

cc @npadmana

Easiest to review side-by-side ignoring whitespace. If you have changes you're working on, feel free to make me rebase.

npadmana commented 5 years ago

This looks good to me -- I had a few questions/thoughts in the review.

I'm doing some large scale hacking of old, dead code, we can figure out the merge as soon as I'm done.

One question -- I'm actually fine with the {x,y}Plan{Sm,Lg} rewrite. We could certainly hide that behind a BatchedFFTWPlan wrapper, but it feels unnecessary here.

ronawho commented 5 years ago

I'm doing some large scale hacking of old, dead code, we can figure out the merge as soon as I'm done.

Cool!

One question -- I'm actually fine with the {x,y}Plan{Sm,Lg} rewrite. We could certainly hide that behind a BatchedFFTWPlan wrapper, but it feels unnecessary here.

I have a different branch that does that, but I didn't want to jam it into this PR. I was going to open up a separate PR to discuss after this went in. https://github.com/npadmana/DistributedFFT/commit/8da6959610269be1da1e157c9754892bee0cfdce if you want to take a look before that.