Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oameye
Copy link

@oameye oameye commented Oct 23, 2024

No description provided.

@@ -1,20 +1,22 @@
name = "EasyFFTs"
uuid = "08be435b-48e7-4090-a646-9e3615ae1968"
authors = ["KronosTheLate"]
version = "0.3.0"
version = "0.4.0"
Copy link
Owner

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

Copy link
Owner

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.

@KronosTheLate
Copy link
Owner

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?

@oameye
Copy link
Author

oameye commented Oct 28, 2024

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 easyfft and response rescaled later.

Now, I very much agree with assessments:

  • Having windowing in the package can offload the scaling of the response afterwards
  • Windowing should not be enabled by default

Regarding DSP dependency, we could extend the easyfft in the with an extension when DSP is loaded. However, this would mean you will have to change the julia compat to at least 1.9. If you don't like the extention idea, I am also okay with the documentation-example.

@KronosTheLate
Copy link
Owner

KronosTheLate commented Oct 30, 2024

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 hanning, which is function, it would actually not even require a package extension! So no need to bump the version ^_^

@KronosTheLate
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants