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

Fft makeover #95

Merged
merged 7 commits into from
Dec 13, 2024
Merged

Fft makeover #95

merged 7 commits into from
Dec 13, 2024

Conversation

Jellevanderwerff
Copy link
Owner

In order:

  • power is now returned (ff44117)
  • dc offset removal has been added as an argument that defaults to True (offset removed; 41cbc1f)
  • Nyquist was already implemented of course
  • I don't see a difference between the two options actually.

@Jellevanderwerff
Copy link
Owner Author

Can be merged (despite test failing, which is to do with an unrelated issue)

@YannickJadoul
Copy link
Collaborator

Huh, sorry, I pulled this and rebased, but something must have gone wrong. Figuring out what went wrong!

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Dec 12, 2024

Ah, I think I didn't fetch before :-|
Cleaning up!

@YannickJadoul
Copy link
Collaborator

OK, back from the depths of git.

Note, still rebased onto main with passing tests.
So update your local branch with git pull --rebase or git reset --hard origin/fft_makeover, @Jellevanderwerff!

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

LGTM!
Couple of suggetsions, but nothing crucial apart from the "first even not at time 0" note

thebeat/stats.py Outdated Show resolved Hide resolved
thebeat/stats.py Show resolved Hide resolved
thebeat/stats.py Outdated Show resolved Hide resolved
@YannickJadoul YannickJadoul merged commit 3ebb59f into main Dec 13, 2024
35 checks passed
@YannickJadoul YannickJadoul deleted the fft_makeover branch December 13, 2024 11:14
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