search

JuliaMath / NFFT.jl

Julia implementation of the Non-equidistant Fast Fourier Transform (NFFT)
Other
153 stars 28 forks source link

remove hardcoded Float64 #95

Closed JakobAsslaender closed 2 years ago

JakobAsslaender commented 2 years ago

Hi, the code added a 0.5 which is by default a Float64 and broke the code when using single precision. I changed this to 1//2 is is a Rational{Int64} that gets promoted appropriately to the type of xscale. Not sure if this is the best solution, we could also just use T(0.5). Let me know what you prefer or if you know of advantages/disadvantages of one over the other approach. -ja

codecov[bot] commented 2 years ago

Codecov Report

Merging #95 (17b21ce) into master (20c7a62) will not change coverage. The diff coverage is 93.33%.

@@           Coverage Diff           @@
##           master      #95   +/-   ##
=======================================
  Coverage   92.35%   92.35%           
=======================================
  Files           8        8           
  Lines         994      994           
=======================================
  Hits          918      918           
  Misses         76       76
Impacted Files Coverage Δ
src/precomputation.jl 91.49% <93.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 20c7a62...17b21ce. Read the comment docs.

JakobAsslaender commented 2 years ago

Never mind, I ran a quick benchmark and T(0.5) seems to be a bit faster than 1//2, so I switched it.

tknopp commented 2 years ago

Thanks, looks good, thanks! Indeed T(0.5) is the right way to do this.

By the way: Your editor makes many whitespace changes. This makes it pretty difficult to review patches, see https://github.com/JuliaMath/NFFT.jl/pull/95/files