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

Unify phasor_from_signal functions #98

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

cgohlke
Copy link
Member

@cgohlke cgohlke commented Aug 8, 2024

Description

This PR merges the phasor_from_signal and phasor_from_signal_fft functions into one:

  • Remove phasor_from_signal_fft function.
  • Add two parameters to phasor_from_signal: use_fft and rfft (used to be rfft_func). By default, FFT is automatically used when all or at least 8 harmonics are calculated, or a rfft function is specified.
  • Change irfft_func parameter to irfft in phasor_to_signal.
  • Change Cython implementation to return NaN or Inf instead of 0.0 in case of zero DC.
  • Document that zero phasor coordinates are legit and phasor coordinates are undefined for zero DC.
  • Update tests.
  • Update benchmark tutorial.

The introduction tutorial is currently failing due to #96 and #97

Release note

Summarize the changes in the code block below to be included in the
release notes:

Unify phasor_from_signal functions

Checklist

  • The pull request title, summary, and description are concise.
  • Related issues are linked in the description.
  • New dependencies are explained.
  • The source code and documentation can be distributed under the MIT license.
  • The source code adheres to code standards.
  • New classes, functions, and features are thoroughly tested.
  • New, user-facing classes, functions, and features are documented.
  • New features are covered in tutorials.
  • No files other than source code, documentation, and project settings are added to the repository.

@cgohlke cgohlke added the enhancement New feature or request label Aug 8, 2024
@cgohlke cgohlke requested a review from bruno-pannunzio August 8, 2024 20:39
@cgohlke cgohlke self-assigned this Aug 8, 2024
Copy link
Contributor

@bruno-pannunzio bruno-pannunzio left a comment

Choose a reason for hiding this comment

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

Excellent @cgohlke! I think this improves the clarity of phasor_from_signal and simplifies the workflow. It all looks good to me (just some minor comments that can be disregarded).

@bruno-pannunzio
Copy link
Contributor

  • Change Cython implementation to return NaN or Inf instead of 0.0 in case of zero DC.
  • Document that zero phasor coordinates are legit and phasor coordinates are undefined for zero DC.

Also I think this is much better than before, so they are not considered for averaging, filtering, etc.

@cgohlke
Copy link
Member Author

cgohlke commented Aug 9, 2024

One issue I noticed is that the function returns scalar values as Python float while the typing says NDArray. MyPy doesn't complain, and it's nice for interactive use and docstring examples, but I think it might be more important to be consistent within the library and numpy.

@cgohlke cgohlke merged commit fb9a7af into phasorpy:main Aug 9, 2024
16 checks passed
@cgohlke cgohlke deleted the unify_phasor_from_signal branch August 9, 2024 15:12
schutyb pushed a commit to schutyb/phasorpy that referenced this pull request Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants