Skip to content

Conversation

@pavyamsiri
Copy link
Contributor

Contributes to completing #99.

I added type stubs to the stub file signal/_spectral_py.pyi and tests for the function overloads I added.

Caveats

I discovered that the function lombscargle was recently updated to include new parameters which also expands the possible return types (see PR scipy/scipy#21277). It also adds inline type stubs which might be an issue.

I did not type stub against this new version though because I think it is still in the dev branch and is not part of a release.

Also I saw that the function periodogram's docs don't specify that None is allowed for the argument window but the body explicitly checks for None. Therefore I allowed None to be passed in as well. This is a bit annoying because the type annotation is no longer consistent with other window arguments in other functions.

I could have perhaps used more shaped typing but its a bit hard to verify because the functions are a bit complex, so I just left things as any shape.

@pavyamsiri pavyamsiri force-pushed the improve/signal/spectral branch from 0d3ff4a to cc8f55a Compare November 4, 2024 02:28
@pavyamsiri pavyamsiri changed the title Improve/signal/spectral signal: Add type stubs to _spectral_py.pyi. Nov 4, 2024
@jorenham jorenham self-requested a review November 4, 2024 09:47
@jorenham jorenham added this to the 1.14.1.3 milestone Nov 4, 2024
@jorenham
Copy link
Member

jorenham commented Nov 4, 2024

I did not type stub against this new version though because I think it is still in the dev branch and is not part of a release.

Yea, that'll be in the scipy 1.15.0 release (see https://discuss.scientific-python.org/t/proposed-release-schedule-for-scipy-1-15-0/1502 for the release schedule).


Also I saw that the function periodogram's docs don't specify that None is allowed for the argument window but the body explicitly checks for None. Therefore I allowed None to be passed in as well. This is a bit annoying because the type annotation is no longer consistent with other window arguments in other functions.

Yea good catch. This is not the first time that the docs are incorrect unfortunately. And I agree with you judgement call here, because as stated in our code of conduct:

Practicality beats purity
Consistency beats practicality
Validity beats consistency


I could have perhaps used more shaped typing but its a bit hard to verify because the functions are a bit complex, so I just left things as any shape.

Yea it's usually better to avoid guessing the return type, and go with the "safe" option. Plus, shape-typing is somewhere at the bottom of the (hypothetical) priority list, so no worries.

Copy link
Member

@jorenham jorenham left a comment

Choose a reason for hiding this comment

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

Nice job on the type-tests! I should probably do that more often as well haha.

I left a couple of small suggestions, but in general this looks very good to me 👌🏻.

@pavyamsiri
Copy link
Contributor Author

I think all the issues should be addressed now.

Copy link
Member

@jorenham jorenham left a comment

Choose a reason for hiding this comment

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

Apart from an issue with float128 and complex256 (which is actually more of an issue with numpy), I'm be happy to merge this; the other comments I leave up to your judgement.

@jorenham jorenham mentioned this pull request Nov 4, 2024
21 tasks
pavyamsiri and others added 5 commits November 5, 2024 08:01
In `_spectral_py.pyi` to be backwards compatible with numpy<2.

Co-authored-by: Joren Hammudoglu <[email protected]>
In `_spectral_py.pyi` to be backwards compatible.

Co-authored-by: Joren Hammudoglu <[email protected]>
@pavyamsiri
Copy link
Contributor Author

Good now?

@jorenham jorenham merged commit a03b355 into scipy:master Nov 4, 2024
2 checks passed
@jorenham
Copy link
Member

jorenham commented Nov 4, 2024

Thanks again, @pavyamsiri 😄

@pavyamsiri pavyamsiri deleted the improve/signal/spectral branch November 4, 2024 21:46
@jorenham
Copy link
Member

jorenham commented Nov 4, 2024

@pavyamsiri Do you think that the "phase" of scipy.signal in the README.md could be updated from 🌒 to 🌓?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants