Repository navigation
Conversation
The docstring still described the single-band signature: it listed nterms, which does not exist here, and omitted bands_fit, nterms_base, nterms_band, reg_base, reg_band and regularize_by_trace. Wording taken from LombScargleMultiband, as suggested in the issue.
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
pllim
added
Docs
Bug
no-changelog-entry-needed
backport-v7.2.x
|
Both done — formatting aligned with the rest of the section, and the change log fragment removed. On the fragment: I added it because the towncrier check was failing, before I noticed you had already applied |
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
Please also fix the grammar in new text, e.g., capitalization and so on. Thanks.
|
Done, and the comma in the type line went with it. I had missed that suggestion when I pushed the capitalization fix. |
dhomeier
left a comment
dhomeier
left a comment
There was a problem hiding this comment.
bands_fit must be array-like, and in fact I believe bands needs to be passed as list of strings when not an array.
The rests corresponds directly to LombScargleMultiband parameters and thus should be fine.
| bands_fit : array-like or str | ||
| Bands to use in the fit, must be a subset of the bands in the input data |
There was a problem hiding this comment.
| bands_fit : array-like or str | |
| Bands to use in the fit, must be a subset of the bands in the input data | |
| bands_fit : array-like | |
| The unique bands to use in the fit, must be a subset of the bands in the input data |
This cannot work with str (see e.g. bands_fit[:, np.newaxis] operation below).
The call from LombScargleMultiband.model() uses "list or array-like", but I don't see how it could work with a plain list either.
BTW it looks like the same should hold for bands above, which is called with LombScargleMultiband.bands (but then processed as np.asarray()).
There was a problem hiding this comment.
@dougbrn if I read the tests right, they are passing np.array(['a', 'aa', 'aaa', ...]) as bands, so that should probably indeed not be a single string?
There was a problem hiding this comment.
You are right, and the str was mine from the first draft. Line 99 does bands_fit[:, np.newaxis], which a plain string will not survive, so the type was wrong no matter what the caller docs say. Applied your wording.
|
Took Left the description as it was, that part reads fine. |
|
Thanks for checking! Even if |
|
Agreed. The docstring says arrays for both now, so there is not much room left to read it the other way. Everything from your review is in as of yesterday, so this one is only waiting on a re-review. |
|
@dhomeier gentle ping on this one, I think everything from your review is in |
Description
Closes #20207.
The multiband
periodic_fitcarried the single-band docstring: it documentednterms, which this function does not take, and left six real parameters undescribed —bands_fit,nterms_base,nterms_band,reg_base,reg_band,regularize_by_trace.Following @dougbrn's suggestion in the issue, the wording comes from
LombScargleMultibandinlombscargle_multiband/core.py, andbands_fituses the phrasing already used elsewhere in the submodule:The entries are ordered to match the signature.
Docstring only — no signature, call site, or behaviour is touched.
AI assistance
Per the astropy AI policy: the mismatch was surfaced by a sweep comparing numpydoc parameter names against the actual signature, run with Claude in Claude Code. I read the signature and the source docstring in
core.pyby hand before writing the entries, and the descriptions are copied from the project's own wording rather than invented.