8000 Narrow the return types of the functions in `fft._helper` · Issue #339 · numpy/numtype · GitHub
[go: up one dir, main page]
Skip to content

Narrow the return types of the functions in fft._helper #339

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.

Sign up for GitHub

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

Jump to bottom
Closed
8000
jorenham opened this issue Mar 19, 2025 · 4 comments · Fixed by #349
Closed

Narrow the return types of the functions in fft._helper #339

jorenham opened this issue Mar 19, 2025 · 4 comments · Fixed by #349
Labels
numpy.fft.* stubs: Enhancement
Milestone
v2.2.6.0

Comments

@jorenham
Copy link
Member

Please describe the feature or change you would like to see:

The current return types are annoying broad, which could lead to issues with external library function signatures with too narrow input types, like matplotlib.

This was originally pointed out by @Jeitan in numpy/numpy#28076 (comment)
@jorenham jorenham mentioned this issue Mar 19, 2025
Open
@Jeitan
Copy link
Contributor
Jeitan commented Mar 20, 2025 •
edited
Loading

Not sure if it's worth a separate issue, but I think I see a typo in what I think is the test: in test\static\accept\fft.pyi, it looks like the tests for fftfreq are repeated twice, when I suspect that the second ones are supposed to be testing rfftfreq. Maybe? If I change the second ones to rfftfreq, at least basedpyright seems fine with it still.

I saw it because I thought I would take a look at this as something possibly within my grasp I could help with, but I fear I may be in over my head. It looks like the stubs for fftfreq are accepting an ArrayLike (and the tests supply an ndarray) for the 'd' parameter, when the documentation clearly states it's a scalar. Is that an implementation detail or a typo? Everywhere else in that module the ArrayLike is used appropriately. If it's a typo I could take a stab at fixing.

@jorenham
Copy link
Member Author

Not sure if it's worth a separate issue, but I think I see a typo in what I think is the test: in test\static\accept\fft.pyi, it looks like the tests for fftfreq are repeated twice, when I suspect that the second ones are supposed to be testing rfftfreq. Maybe? If I change the second ones to rfftfreq, at least basedpyright seems fine with it still.

Good catch! A PR for this is certainly welcome welcome :)

I saw it because I thought I would take a look at this as something possibly within my grasp I could help with, but I fear I may be in over my head.

Yea I took a look, and it's indeed not as easy as it looks. Array-like's are notoriously difficult to properly type, and in this case, there's also shape-typing involved. I'll work on this soon, and will ping you once I have the PR ready, in case you're interested.

It looks like the stubs for fftfreq are accepting an ArrayLike (and the tests supply an ndarray) for the 'd' parameter, when the documentation clearly states it's a scalar.

Yea the documentation is not the whole truth in this case. Because in practice it'll work with anything that's broadcastable with a 1d array of shape (n,), i.e.

>>> import numpy as np
>>> np.fft.fftfreq(4, np.linspace(.1, .2, 8).reshape(2, 4))
array([[ 0.        ,  2.1875    , -3.88888889, -1.75      ],
       [ 0.        ,  1.45833333, -2.69230769, -1.25      ]])

So if d is a scalar-like, it'll return a 1d array, and otherwise, it'll return an array with the same shape as d (s.t. np.shape(d)[-1] == n).

@Jeitan
Copy link
Contributor
Jeitan commented Mar 20, 2025

Yea the documentation is not the whole truth in this case. Because in practice it'll work with anything that's broadcastable with a 1d array of shape (n,), i.e.

Ah, I thought it might be something like that. I will leave it to you then because I definitely don't know the code well enough heh.

I can fix the test typo though!

@jorenham jorenham added this to the v2.2.x.0 milestone Mar 20, 2025
@Jeitan
Copy link
Contributor
Jeitan commented Mar 20, 2025

I made PR #346 for the typo :-)

This was referenced Mar 20, 2025
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
numpy.fft.* stubs: Enhancement
Projects
None yet
Milestone
v2.2.6.0
Development

Successfully merging a pull request may close this issue.

✨ shape-typing and improved dtypes for numpy.fft.[i]fftshift numpy/numtype
2 participants
@jorenham @Jeitan
0