Skip to content

Multiband periodic_fit documents nterms, but takes nterms_base and nterms_band #20207

Description

@darkdi

Following up on #20205, where @pllim asked me to open one issue per remaining problem. Different subpackage from #20206.

periodic_fit in the multiband Lomb-Scargle MLE implementation documents nterms:

def periodic_fit(
    t, y, dy, bands, frequency, t_fit, bands_fit,
    center_data=True,
    nterms_base=1,
    nterms_band=1,
    ...
):
    """...
    nterms : int (default=1)
        The number of Fourier terms to include in the fit

There is no nterms parameter. The function takes two separate term counts, nterms_base and nterms_band, and neither is documented — so a reader following the docstring would pass nterms=... and get a TypeError, while the two knobs that do exist are invisible.

Looks like the docstring was carried over from the single-band periodic_fit, which does take a single nterms.

The fix would be to replace the entry with the two real parameters, but the wording for each depends on what the multiband model does with them (base terms shared across bands vs. per-band terms), so I would rather not guess at the description. Happy to send a PR if you tell me how you would phrase the two.

Found with a static checker comparing numpydoc parameter names against the signature below them; each finding was read by hand.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions