-
Notifications
You must be signed in to change notification settings - Fork 4
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
add window option to easyffts
#14
base: main
Are you sure you want to change the base?
Conversation
@@ -1,20 +1,22 @@ | |||
name = "EasyFFTs" | |||
uuid = "08be435b-48e7-4090-a646-9e3615ae1968" | |||
authors = ["KronosTheLate"] | |||
version = "0.3.0" | |||
version = "0.4.0" |
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.
I am rather sure that for versions 0.X.Y, incrementing X means that the change is breaking. It is only after the 1.X release that you increment the X for non-breaking changes. So this should be 3.1
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.
Before changing this, please see my comment on the larger picture.
Hey, thanks for making a PR! Sorry for taking so long to get it checked out. While it is often a good idea to do windowing, it seems like a very opinionated step that can be added before computing the FFT. The only benefit I see is that there is some extra details about scaling the result, and doing that for the user is very much in line with the spirit of this package. Is this your thought? That it is cumbersome to get the rescaling right when windowing? Also, applying windowing by default seems like a very bad idea to me, as I see no reason to expect it as a user. It would make this package go beyong adding convenience, and actually alter the results. Finally, taking on DSP as a dependency is rather significant, which makes this package much much less lightweight. This is my biggest issue with this PR. All in all, I feel like the cons outweight the pros, and that most of the benefit (making it easier to scale correctly when windowing) could be provided by writing up a copy-pastable example in the docs (readme) showing how to do it. Does providing a documentation-example feel like a decent compomise to you? |
Hey, thank you for your extensive response! To be honest I didn't lay so much thought on the changes as you did. I needed windowing and quickly made the changes on top of you package. I thought, it could be useful to add the functionality to the package. I realise now that indeed the windowing could just be applied outside Now, I very much agree with assessments:
Regarding DSP dependency, we could extend the |
Seeing that the LTS is actually 1.10, I would actually be fine in bumping the version to 1.10. We could then have two signatures: One as it is now, and one that takes a function as the second argument. If a function is provided, it is assumed to be a window function, which returns a window when given the length of the signal (or, in the multidimentional case, the length along each dimension as a tuple). But then I realize: Because this would dispatch on the type of |
I put something together in #15. Does that look good to you? I am pretty sure that you can uninstall the package, and install it again targeting the https://github.com/KronosTheLate/EasyFFTs.jl/tree/add_windowing_convenience branch to test it locally if you want. |
No description provided.