search

npadmana / DistributedFFT

6 stars 2 forks source link

More elegance cleanup #34

Closed ronawho closed 4 years ago

ronawho commented 4 years ago
ronawho commented 4 years ago

@npadmana Misc questions for other potential cleanups

npadmana commented 4 years ago

@ronawho --

  • I don't think isValid is used. Could maybe be removed, but seemed worth keeping (or moved into fftw module?)

Yes, I thought about deleting in in my last cleanup. And then decided against it, because I thought it might be useful to have. I agree, it should ideally be in the FFTW module as a test. Also, it would be nice to avoid the separate C code for this -- it would be nice to access NULL using the C interop features.

Let's leave this in for now, but maybe we should open Chapel issues for these.

  • I think setup1DPlan could just call into setupPlanColumns

I'm not quite sure. For the strided version, yes -- but for the unstrided version, I'm not sure. I'd be in favor of shelving this one for now.

  • I wondered if the cleanup called in the module deinit should be in the fftw module

Yes, it should (and in fact, it's in an issue chapel-lang/chapel#5751). Ditto with some of the record plan stuff. However, I wanted to defer doing those to a later date when we thought a little bit more about exactly how we should set that up.

npadmana commented 4 years ago

These look good. I think after we get through our list of merges, we should probably rerun the perf suite again, just to make sure we haven't regressed somewhere.

I'm ready to merge this, if you are.

ronawho commented 4 years ago

These look good. I think after we get through our list of merges, we should probably rerun the perf suite again, just to make sure we haven't regressed somewhere.

Yeah, I ran size D/E up to 256 nodes, but was planning on doing full runs once we were done.

I'm ready to merge this, if you are.

Yup

ronawho commented 4 years ago

I have one last potential change, and I should be done after that. Working on the PR now.