Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

signal: Add type stubs to _spectral_py.pyi. #157

Merged
merged 18 commits into from
Nov 4, 2024

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
Owner

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
Owner

@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 👌🏻.

scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
tests/signal/test_spectral.pyi Outdated Show resolved Hide resolved
tests/signal/test_spectral.pyi Outdated Show resolved Hide resolved
@pavyamsiri
Copy link
Contributor Author

I think all the issues should be addressed now.

Copy link
Owner

@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.

scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Outdated Show resolved Hide resolved
scipy-stubs/signal/_spectral_py.pyi Show resolved Hide resolved
@jorenham jorenham mentioned this pull request Nov 4, 2024
21 tasks
pavyamsiri and others added 5 commits November 5, 2024 08:01
Co-authored-by: Joren Hammudoglu <jhammudoglu@gmail.com>
In `_spectral_py.pyi` to be backwards compatible with numpy<2.

Co-authored-by: Joren Hammudoglu <jhammudoglu@gmail.com>
In `_spectral_py.pyi` to be backwards compatible.

Co-authored-by: Joren Hammudoglu <jhammudoglu@gmail.com>
@pavyamsiri
Copy link
Contributor Author

Good now?

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

jorenham commented Nov 4, 2024

Thanks again, @pavyamsiri 😄

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

jorenham commented Nov 4, 2024

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

@jorenham jorenham added the stubs: improvement Improve or refactor existing annotations label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scipy.signal stubs: improvement Improve or refactor existing annotations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants