Minor type stability and allocation improvements #52
Conversation
Note that Array{T} is not a concrete type; Array{T,N} is.
There was a problem hiding this comment.
Hi! Thank you for contributing, and apologies for not getting back to you earlier. There is not much development on this interface at the moment, and I must have missed the PR notification.
Nice catch of allocations and instabilities, optimizations are always welcome even if on the margin :)
| @@ -11,9 +11,10 @@ finufft_jll = "2.1.0" | |||
| julia = "1.3" | |||
|
|
|||
| [extras] | |||
| JET = "c3a54625-cd67-489e-a8e7-0a5a0ff4e31b" | |||
There was a problem hiding this comment.
I notice that JET is experimental with behaviour that may vary between Julia versions. Is it safe to depend on, even if just in a test?
| @@ -466,7 +466,7 @@ function nufft1d1!(xj :: Array{T}, | |||
| (ms, ntrans_fk) = get_nmodes_from_fk(1,fk) | |||
|
|
|||
| checkkwdtype(T; kwargs...) | |||
| plan = finufft_makeplan(1,[ms;],iflag,ntrans,eps;dtype=T,kwargs...) | |||
| plan = finufft_makeplan(T,1,[ms;],iflag,ntrans,eps;kwargs...) | |||
There was a problem hiding this comment.
Is this change necessary considering that old interface still exists?
I think there is a point in the code using the documented interface.
| function finufft_makeplan(type::Integer, | ||
| function finufft_makeplan(type::Integer, args...; dtype=Float64, kwargs...) | ||
| finufft_makeplan(dtype, type, args...; kwargs...) | ||
| end |
There was a problem hiding this comment.
Is there any measurable benefit from this change? If I understand it correctly, it allows the compiler to move the if dtype==Float64 from the function body to the multiple dispatch system?
If this is useful, I think that the helper function should be internal and have a unique name so that it doesn't clutter the exported namespace. Also, collecting the arguments into args... hides useful information from the data shown by methods(finufft_makeplan)
First of all, thank you for this amazing library and for providing a Julia interface.
This PR is to propose a few minor changes with the aim of suppressing some tiny memory allocations which occur when performing transforms using the guru interface. When performing many transforms with the same plan, these allocations can add up, and it may be nice to have them removed.
The changes completely suppress allocations when applying a plan using
finufft_setpts!andfinufft_exec!. This is in part achieved by fixing some type instabilities, and also by avoiding the allocation of some empty arrays (as inT[]). Some tests are also included.For convenience, the PR introduces an undocumented variant of
finufft_makeplanwhich takesdtypeas its first positional argument and which is type stable (in the sense that the dtype,Float32orFloat64, is known by the compiler). But one could change the name of this variant if this is confusing.