-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve type inference in plan_r2r
#253
Conversation
Codecov ReportBase: 61.28% // Head: 64.50% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #253 +/- ##
==========================================
+ Coverage 61.28% 64.50% +3.21%
==========================================
Files 5 5
Lines 483 493 +10
==========================================
+ Hits 296 318 +22
+ Misses 187 175 -12
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Test failure in "CI / Julia nightly - macOS-latest - x64 - provider mkl - 1 thread(s)" is unrelated, and is also seen on other recent PRs. |
plan_r2r
Gentle bump |
1 similar comment
Gentle bump |
The |
@stevengj how would you recommend proceeding? I don't know enough about this package to make large changes |
Gentle bump @stevengj |
Gentle bump |
I've simplified the implementation considerably by storing |
flags::UInt32 # planner flags | ||
region::G # region (iterable) of dims that are transormed | ||
kinds::K | ||
pinv::ScaledPlan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we parameterize this? ScaledPlan
is an abstract type, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is, but it's a bit tricky to do this properly, as the struct is initialized incompletely. Maybe I'll look into this in a separate PR
LGTM. |
Co-authored-by: Steven G. Johnson <[email protected]>
Unfortunately, this broke Hadamard.jl (which relies on internals of FFTW.jl); I'll fix it. |
Updated PR
Instead of storing the value of
kinds
as a type parameter inr2r
transforms, we may store it as a struct field instead, as this improves type-stability. This doesn't rely on the value being constant propagated.Older version of this PR
This is a somewhat hacky approach to improve type-inference in
plan_r2r
, in the case wherekinds
andregion
are compile-time constants. This is a common use case, so, for example, the following is type-stable after this:The idea is that, in this case, the second type parameter of
r2rFFTWPlan
may be evaluated at compile time instead of a runtime call converting aVector
to aTuple
. This requires aggressive constant propagation throughBase.@constprop :aggressive
, which is not a part of the public API, but will hopefully be stable.