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

ENH: Add STFT function to Function class #620

Merged
merged 23 commits into from
Aug 18, 2024
Merged

ENH: Add STFT function to Function class #620

merged 23 commits into from
Aug 18, 2024

Conversation

AdvaitChandorkar07
Copy link
Contributor

@AdvaitChandorkar07 AdvaitChandorkar07 commented Jun 12, 2024

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Lint (black rocketpy/ tests/) has passed locally

Current behavior

STFT feature was requested in issue #308 with link to #292 as example

New behavior

Added STFT

@AdvaitChandorkar07 AdvaitChandorkar07 requested a review from a team as a code owner June 12, 2024 18:31
@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to develop June 12, 2024 19:41
@Gui-FernandesBR Gui-FernandesBR added the Enhancement New feature or request, including adjustments in current codes label Jun 12, 2024
@Gui-FernandesBR Gui-FernandesBR changed the title Add STFT function to Function class ENH: Add STFT function to Function class Jun 12, 2024
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Jun 12, 2024
@Gui-FernandesBR Gui-FernandesBR linked an issue Jun 12, 2024 that may be closed by this pull request
@Gui-FernandesBR
Copy link
Member

@AdvaitChandorkar07 thank you so much for your contribution! We are going to review your PR as soon as possible.

At a first glance, I think it solves the problem as requested by #308 . Good job!

rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Show resolved Hide resolved
@Lucas-Prates
Copy link
Contributor

Lucas-Prates commented Jul 11, 2024

Thank you for your contribution, @AdvaitChandorkar07, and sorry for the long delay for a review.
The code itself is well-written and clear. For the actual results, we need a more suitable example to compare the
implementation. I compared the results of your implementation with SciPy's SFTF example. I had to adapt it to use the boxcar window function to match your implementation. Here are the results of the spectrograms:
rocketpy_stft
scipy_stft

The results look very similar, although there were some minor differences (check the colors around 25s and at the beginning and end of the signal). One major difference is that the the SciPy FFT magnitude seems to be half of the FFT magnitude computed by RocketPy. Here is the very simple notebook I used to compare SciPy to your implementation. testing_stft.zip

Overall, things look very good. Some final comments:
(1) We should investigate why the FFT magnitude of SciPy seems to be half of our implementation. Maybe I did something wrong in the examples or computing the absolute values?
(2) Do you have an interest in implementing different window functions? For instance, the SciPy STFT win argument is another function that is multiplied by the signal in each window. However, this would make the implementation more complicated, so I think it's okay to leave this as a feature for the future;
(3) Adapting the example as was mentioned in the review would help a lot in understanding how useful the STFT is.

rocketpy/mathutils/function.py Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to figure out what is happening with the tests on the CI.

But tests are passing locally, already a good sign.

@Gui-FernandesBR Gui-FernandesBR merged commit e40eecc into RocketPy-Team:develop Aug 18, 2024
5 checks passed
@Gui-FernandesBR
Copy link
Member

Thanks a lot for the contribution, @AdvaitChandorkar07 !! Great work.

juliomachad0 pushed a commit that referenced this pull request Aug 24, 2024
Squash of the following commits:
* Add STFT function to Function class
* Added feature: Short-Time Fourier Transform function
* Added feature: Short-Time Fourier Transform function
* "Variable name changes in stft"
* "Variable and function name formatting"
* "Better Example"
* Add STFT function to Function class
* Added feature: Short-Time Fourier Transform function
* Added feature: Short-Time Fourier Transform function
* "Variable name changes in stft"
* "Variable and function name formatting"
* "Better Example"
* Fixed the doctest
* "Spectrogram example"
* small fixes to STFT function

---------

Signed-off-by: AdvaitChandorkar07 <[email protected]>
Co-authored-by: Gui-FernandesBR <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request, including adjustments in current codes
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

ENH: Implement time-frequency analysis methods
3 participants