Precompute FFTW plan #159
Merged
Precompute FFTW plan #159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
giordano
force-pushed
the
mg/fft-plan
branch
6 times, most recently
from
8f7cefe
to
c64d531
Compare
giordano
commented
Mar 5, 2021
giordano
force-pushed
the
mg/fft-plan
branch
3 times, most recently
from
5e8df32
to
96fbec3
Compare
|
Ready for review but not for merging, I'll need to revert the change to the benchmark before that |
tkoskela
reviewed
Mar 8, 2021
| # Operates on the 2d data stored as a matrix. | ||
| # From Dietrich & Newsam 96 in text following equation 12 | ||
| function normalized_inverse_2d_fft!(transformed_array::AbstractMatrix{T}, array::AbstractMatrix{S}, grid_ext) where {T,S} | ||
| function normalized_2d_fft!(transformed_vector::AbstractVector{<:Complex}, vector::AbstractVector{S}, fft_plan::FFTW.FFTWPlan, fft_plan!::FFTW.FFTWPlan, grid_ext, f=identity) where {S} |
There was a problem hiding this comment.
That's correct, but all these methods need to have the same signature in terms of number of arguments.
Comment on lines
+149
to
+151
| mul!(transformed_array, f(fft_plan!), array) | ||
| else | ||
| mul!(transformed_array, f(fft_plan), array) |
There was a problem hiding this comment.
What's the difference between fft_plan and fft_plan!?
There was a problem hiding this comment.
The out-of-place fft_plan can be used only in
mul!(B, fft_plan, A)with A and B different arrays, or more precisely pointer(A) != pointer(B): https://github.com/JuliaMath/FFTW.jl/blob/2c20abe7098a2380fa37fe021b3d21fc593059ec/src/fft.jl#L438. Instead, the in-place fft_plan! can only be used in
mul!(A, fft_plan!, A)We do have cases where we overwrite the input array.
| normalization_factor = 1.0 / sqrt(grid_ext.nx * grid_ext.ny) | ||
| transformed_array .= fft(array) .* normalization_factor | ||
| normalization_factor = f(sqrt(grid_ext.nx * grid_ext.ny)) | ||
| if pointer(transformed_array) == pointer(array) |
There was a problem hiding this comment.
See the answer to #159 (comment)
Comment on lines
+372
to
+374
| fft_plan, fft_plan! = FFTW.plan_fft(tmp_array), FFTW.plan_fft!(tmp_array) | ||
|
|
||
| offline_matrices = init_offline_matrices(grid, grid_ext, stations, model_noise_params, obs_noise_std, T) | ||
| offline_matrices = init_offline_matrices(grid, grid_ext, stations, model_noise_params, obs_noise_std, fft_plan, fft_plan!, T) |
There was a problem hiding this comment.
It looks like fft_plan, fft_plan! are always passed together so could just store them as a tuple?
There was a problem hiding this comment.
This is actually what I did at the beginning, but I didn't like it too much because it was opaque what fft_plan[1] and fft_plan[2] were (which is the out-of-place, which is the in-place?). I guess you'd suggest a named tuple? 😄
tkoskela
added a commit
that referenced
this pull request
Mar 17, 2021
* Add functions for Alex optimal filter * Rewrite functions with rho_bar a vector and R21,R12 matrices * sample_height_proposal passes preliminary tests * sample_height_proposal passes preliminary tests * Add validation against Alex code * Add performance benchmarks * Shift grid indices so that both station and grid indexing starts at 1. Shift indices when initialising StationVectors from st.st_ij of Sample_Optimal_Height_Proposal.jl * Add get_log_weights function * Add comments, fix bugs change i to iprt for correct behaviour in get_log_weights! interpolate variables in function calls that are timed with @Btime * Apply suggestions from code review Add dots and in-place operations Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * matrices.K*KH is a real matrix product * Use mul! to do multiplication in-place * Remove duplicate call * Minimal allocations reduction * Add get_stations and set_particles! to the interface * Reorder data structures for compatibility. Remove unit tests (will move to runtests.jl) * Integrate Optimal Filter to ParticleDA - Added unit tests and validation tests - Made changes to model interface to expose fields needed by optimal filter - Added a OptimalFilter type for dispatching - No implementation of run_particle_filter yet * Apply suggestions from code review Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Draft implementation * Changes suggested by review Changing rand(rng,nd) to randn(rng) causes validation test to fail unless it's done on both sides. Unclear why. * Implementation according to comment on 5 Feb in #136 * Add compat entry for FFTW * Change to a more sensible test height field * Make grid size nx (not nx+1) on both ParticleDA and OptimalFilter. Something goes wrong. * Correct dx,dy in covariance calculations. tests pass. * Fix bugs * Apply suggestions from code review Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Apply suggestions from code review In two parts because I didn't realize github doesn't automatically load a large diff even when there are comments. Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Define model grid as storing values at the nodes (instead of cell faces) The only change required is defining dx as L_x/(nx-1) instead of L_x/nx In my opinion this is more consistent with the way we define the stations Makes it easier (or perhaps even possible) to integrate with the optimal filter Also fixed a bug in write_stations that created an Int dataset on the hdf5 file for the coordinate (float) value Reference data and some tests had to be updated * Pass grid parameters instead of using model_params * Fix bugs * Minor improvements - Parametric types in init_filter - Comments - Indentations * Fix arguments to init_*line_matrices * Use sigma and lambda from model params instead of redefining them in filter. * update notebook with animation and parameters * Point out bootstrap filter in example * add FFTW to benchmark environment * Pass filter type to run_particle_filter and use it to dispatch (#155) * Pass filter type to run_particle_filter and use it to dispatch appropriate functions * Update to simpler syntax * Update benchmarks and the benchmark environment Co-authored-by: Mosè Giordano <mose@gnu.org> * Split get_grid_size into three functions (#157) * Split get_grid_size into three functions that return NTuples * Remove extra parenthesis * Clarify function names * Changes suggested by review - Define Grid in OptimalFilter - Annotate argument types - Fix runtests.jl - Fix docstrings * Add integration test with optimal filter * Add input file for the test * Precompute FFTW plan (#159) * Precompute FFTW plan * Support in-place and out-of-place FFTs * Reduce duplication in `normalized_2d_fft!` methods * Force A to be hermitian to avoid roundoff errors Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Throw error if matrix is not approximately Hermitian * clean up * Pass RNG directly to the filter instead of the model Also, use the RNG also in the `resample!` function. * Reduce allocations in `sample_height_proposal!` (#162) Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> Co-authored-by: Mosè Giordano <mose@gnu.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Very work-in-progress (still needs to adapt tests)