Skip to content

Fix n_fft_samples docstring: uses next_fast_len, not next power of 2#74

Merged
edeno merged 2 commits into
masterfrom
copilot/fix-multitaper-docstring
Nov 3, 2025
Merged

Fix n_fft_samples docstring: uses next_fast_len, not next power of 2#74
edeno merged 2 commits into
masterfrom
copilot/fix-multitaper-docstring

Conversation

Copilot AI commented Nov 1, 2025

Copy link
Copy Markdown
Contributor

The Multitaper class docstring incorrectly stated that n_fft_samples defaults to "next power of 2 >= n_time_samples_per_window". The implementation actually uses scipy.fft.next_fast_len() (or cupy.fft.next_fast_len() for GPU), which returns an FFT-efficient length that may not be a power of 2.

For example:

from scipy.fft import next_fast_len
next_fast_len(30000)  # Returns 30000, not 32768
next_fast_len(100)    # Returns 100, not 128

Changes

  • Updated n_fft_samples parameter docstring in Multitaper.__init__() to accurately describe the behavior:
    • Removed incorrect "next power of 2" claim
    • Added reference to scipy.fft.next_fast_len / cupy.fft.next_fast_len
    • Clarified that the value is chosen for FFT efficiency, not strictly power-of-2 alignment
Original prompt

This section details on the original issue you should resolve

<issue_title>multitaper DocString incorrect</issue_title>
<issue_description>the Multitaper constructor says:
n_fft_samples : int, optional Length of FFT. If None, uses next power of 2 >= n_time_samples_per_window.

But e.g. if the data length is 30000, it gives 30000. This is because cupy.fft.next_fast_len(30000)=30000. I'm not sure why it thinks that 30000 is a fast length. From its docstring: "note::
It may return a different value to :func:scipy.fft.next_fast_len
as pocketfft's prime factors are different from cuFFT's factors.
For details, see the cuFFT documentation_."

Maybe we should change the docstring to say:
n_fft_samples : int, optional Length of FFT. If None, uses a value >= n_time_samples_per window chosen to be fast for the FFT algorithm, cupy.fft.next_fast_len(n_time_samples_per_window).</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: edeno <8053989+edeno@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix incorrect multitaper docstring for n_fft_samples Fix n_fft_samples docstring: uses next_fast_len, not next power of 2 Nov 1, 2025
Copilot AI requested a review from edeno November 1, 2025 17:32
@edeno edeno requested a review from emilyps14 November 1, 2025 21:18
@edeno edeno requested a review from Copilot November 3, 2025 13:09
@edeno edeno marked this pull request as ready for review November 3, 2025 13:09
@codecov

codecov Bot commented Nov 3, 2025

Copy link
Copy Markdown

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the documentation for the n_fft_samples parameter in the Multitaper class to provide more accurate and detailed information about how the FFT length is determined when not explicitly specified.

  • Updated the n_fft_samples parameter description to clarify that it uses scipy.fft.next_fast_len (or cupy.fft.next_fast_len for GPU) instead of simply the next power of 2

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@edeno edeno merged commit a5d2983 into master Nov 3, 2025
27 of 28 checks passed
@edeno edeno deleted the copilot/fix-multitaper-docstring branch November 3, 2025 13:29
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.

multitaper DocString incorrect

4 participants