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

Sliding window changes #33

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

Sliding window changes #33

wants to merge 5 commits into from

Conversation

mibur1
Copy link

@mibur1 mibur1 commented Feb 14, 2025

Hi @mtorabi59

I implemented the changes as discussed in #31.

The changes are:

  • Removed extending the window which has two effects:
    • Bugfix regarding the zero-padding for rectangular SW calculation
    • Limits the tapered window to contain no samples outside the window width
  • Added a window_std parameter to Sliding Window, Discrete HMM, and Sliding Window Clustering
    • Default is None, which results to the original std of 3 * W / 22
  • Disabled the convolution and instead directly use the Gaussian
    • In my opinion this is unnecessary now, as the window size is a hard limit for the window
  • Vectorized Pearson correlation calculation for improved performance

Feel free to modify this to your liking. Also I did not write any tests/documentation, please let me know if I should do so :)

@mtorabi59 mtorabi59 self-requested a review February 17, 2025 22:02
@mtorabi59
Copy link
Collaborator

Thanks @mibur1 ! Overall it looks good! Thank you for making the changes. There a few comments below:

@mtorabi59
Copy link
Collaborator

@mibur1 lmk if you have any Qs!
Also, tests are not necessary, but if you have some ideas for testing the sliding window, it'd be great to have them!

@mibur1
Copy link
Author

mibur1 commented Feb 18, 2025

@mtorabi59 Thanks for the quick review and comments, I hope I addressed all of them. I will think about tests for the sliding window and add some if I have an idea.

@mtorabi59
Copy link
Collaborator

@mibur1 Thanks, everything looks good! There might only be some formatting changes by black or flake8.

@mtorabi59
Copy link
Collaborator

@mibur1 If you're familiar with pre-commit, it would be great to run it locally and make the required formatting changes, ow I'll fix them!

@mibur1
Copy link
Author

mibur1 commented Feb 20, 2025

Thanks! I'll try to do this as well as the unit tests next week. But please feel free to do it yourself if you would like to have it done earlier.

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