-
Notifications
You must be signed in to change notification settings - Fork 57
Make FFTW flags "public" #329
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #329 +/- ##
=======================================
Coverage 73.64% 73.64%
=======================================
Files 5 5
Lines 535 535
=======================================
Hits 394 394
Misses 141 141 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I think all of the flags here should be Lines 70 to 79 in 08e409c
|
|
That's kind of what I guessed- thanks. Done. |
src/FFTW.jl
Outdated
| export dct, idct, dct!, idct!, plan_dct, plan_idct, plan_dct!, plan_idct! | ||
|
|
||
| # FFTW flags from fft.jl | ||
| public MEASURE |
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.
In case it wasn't clear, public doesn't work in v1.10.
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.
Thanks for the reminder. I added a VERSION switch.
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.
Having public within an if statement did not work; i.e., this fails with the error "Expected end" in v1.12.3 on MacOS:
if true
public foo
endFortunately, having an include within the if statement appears to work, so I isolated the public assertions to a separate file, public.jl, as a workaround. Suggestions welcome.
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.
You can use something like https://github.com/JuliaStats/StatsAPI.jl/blob/ee9906ddd8cab08b454d1f0088c5285ac7bed1fe/src/StatsAPI.jl#L5-L10. Or Compat.jl which provides a macro for this expression, but in StatsAPI it didn't seem worth adding an additional dependency (in particular due to the large number of packages that depend on StatsAPI).
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.
Thanks @devmotion for that very helpful suggestion. I'm giving it a try now.
|
Now the |
|
Now passes all the relevant tests. (The macOS-13 tests seem to be deprecated - unrelated to this PR.) |
The documentation for AbstractFFTs makes explicit reference to certain FFTW flags, so it would be very desirable for those flags to be
publicin FFTW so that developers can count on them indefinitely and so that users (like me) who follow those docs do not get warnings about using "non-public" names from ExplicitImports.jl.Perhaps more (all?) of the FFTW constants should also be
publicand you or I could amend this PR to be more inclusive. For now I am limiting the request to just those that are cited explicitly in public Julia package documentation because I am unsure which of the manyconstvalues in fft.jl should be public.