make_cosine_basis(sampling_freq=) takes a sampling interval in seconds, not a frequency
- Dominant language
- Python
- Stars
- 130
- Forks
- 48
- Avg merge
- 6h 42m
- Merged PRs (30d)
- 34
Description
`nltools.algorithms.make_cosine_basis(nsamples, sampling_freq, filter_length, ...)` reads `sampling_freq` as the sampling interval in seconds (the TR), not a frequency in Hz: the SPM-style formula it implements is `fix(2 * n * RT / HParam + 1)`, and `DesignMatrix.add_dct_basis` passes `1 / sampling_freq` to it, which is why the design-matrix path is correct. A user calling the standalone function with a real frequency (0.5 Hz for a 2 s TR) gets the wrong number of regressors.
The #535 sweep (finding G-08) kept the arithmetic as v0.5.1 had it, because flipping the unit of a user-facing keyword is a breaking change, and corrected the docstring to say the argument is a sampling interval in seconds. That is a poor resting state: the keyword name says the opposite of what it takes.
Options for 0.6.1: rename the parameter to `sampling_interval` with a deprecation shim; or make the standalone function take a real frequency and have `add_dct_basis` pass `sampling_freq` straight through, with a migration row. Either needs a decision and a migration row; the S-G verification report holds the reproduction.
Contributor guide
Research direction
Start with the S-G verification report's reproduction, then inspect make_cosine_basis and DesignMatrix.add_dct_basis to compare their sampling units. Resolve which API direction to adopt, verify the standalone and design-matrix behavior, and add the required migration row.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100