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

Improve private parse_harmonic function #138

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

cgohlke
Copy link
Member

@cgohlke cgohlke commented Oct 4, 2024

Description

This PR proposes the following changes to the private _utils.parse_harmonic function:

  1. Replace samples with harmonic_max parameter to make the function more consistent and easier to understand.
  2. Make harmonic_max parameter optional to allow for cases where the upper harmonics limit is unknown (as in Simplify multiple harmonic calibration #124).

This simplifies existing code a little.

Also added a note to the phasor_from_signal function that "A minimum of harmonic * 2 + 1 samples are required along axis to calculate correct phasor coordinates at harmonic". This is actually not verified in parse_harmonic and Fourier transform does return second harmonic numbers for 4 samples even though they are not correct phasor coordinates (as far as I understand). There is however a verified requirement for an absolute minimum of three signal samples.

Checklist

  • The pull request title 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 Oct 4, 2024
@cgohlke cgohlke self-assigned this Oct 4, 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.

Thanks @cgohlke I think this modification is what was needed. I will incorporate this in #124.

@cgohlke cgohlke merged commit 00dc2d0 into phasorpy:main Oct 4, 2024
14 checks passed
@cgohlke cgohlke deleted the parse_harmonic branch October 4, 2024 17:07
schutyb pushed a commit to schutyb/phasorpy that referenced this pull request Jan 16, 2025
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