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.

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

Add type stubs to scipy/signal/_peak_finding.pyi #87

Merged
merged 22 commits into from
Oct 20, 2024
Merged

Add type stubs to scipy/signal/_peak_finding.pyi #87

Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d5c528b
`signal`: Add type stubs to `argrel*` functions
pavyamsiri Oct 16, 2024
c7733bc
`signal`: Add type stubs to `peaks_*` functions
pavyamsiri Oct 16, 2024
88ea09c
`signal`: Add type stubs for `find_peaks` and `find_peaks_cwt`
pavyamsiri Oct 16, 2024
d53d739
`signal`: Fix the type stub for `plateau_size` in `find_peaks`
pavyamsiri Oct 16, 2024
731c4d4
`signal`: Use `total=False` on `TypedDict` to avoid using `NotRequired`
pavyamsiri Oct 17, 2024
b453d06
`signal`: Adjust the function argument list formatting
pavyamsiri Oct 17, 2024
d88bb48
`signal`: Use type aliases for common NDArray types
pavyamsiri Oct 17, 2024
13d8de6
`signal`: Use `_ArrayLikeInt_co` for `peaks`
pavyamsiri Oct 17, 2024
73711bc
`signal`: Use `op.CanIndex` instead `int` for axis
pavyamsiri Oct 17, 2024
77c6017
`signal`: Use `scipy._typing`'s `AnyReal and `AnyInt`
pavyamsiri Oct 17, 2024
cfbae52
`signal`: Specify that a 1D array is returned for `find_peaks_cwt`
pavyamsiri Oct 17, 2024
2266ca0
`signal`: Loosen type constraint on `wavelet`
pavyamsiri Oct 17, 2024
beda31b
`signal`: Use ArrayLike type aliases from numpy instead of NDArrays
pavyamsiri Oct 17, 2024
85f19ce
`signal`: Adjust type annotations for `find_peaks_cwt`
pavyamsiri Oct 19, 2024
17930b7
`signal`: Use `AnyInt` for `order` arguments
pavyamsiri Oct 19, 2024
d7b9cbd
`signal`: Adjust type annotation for `wavelet` argument
pavyamsiri Oct 19, 2024
5e1d325
`signal`: Remove unused import in `_peak_finding.pyi`.
pavyamsiri Oct 19, 2024
09b3644
Merge branch 'master' into find_peaks
pavyamsiri Oct 19, 2024
28fad98
`signal`: Use a bounded `TypeVar` for the comparator function argument
pavyamsiri Oct 19, 2024
11fa2aa
`signal`: Adjust type annotations for `find_peaks_cwt`
pavyamsiri Oct 20, 2024
bb24cad
`signal`: Change type annotation for `find_peaks_cwt`
pavyamsiri Oct 20, 2024
9f1ca74
`signal`: Make `max_distances` an NDArray
pavyamsiri Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 80 additions & 31 deletions scipy-stubs/signal/_peak_finding.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,86 @@
from scipy._typing import Untyped
from collections.abc import Callable, Sequence
from typing import Concatenate, Literal, TypeAlias, TypedDict

import numpy as np
import numpy.typing as npt
import optype as op
from numpy._typing import _ArrayLikeInt_co
from scipy._typing import AnyInt, AnyReal

__all__ = ["argrelextrema", "argrelmax", "argrelmin", "find_peaks", "find_peaks_cwt", "peak_prominences", "peak_widths"]

def argrelmin(data: Untyped, axis: int = 0, order: int = 1, mode: str = "clip") -> Untyped: ...
def argrelmax(data: Untyped, axis: int = 0, order: int = 1, mode: str = "clip") -> Untyped: ...
def argrelextrema(data: Untyped, comparator: Untyped, axis: int = 0, order: int = 1, mode: str = "clip") -> Untyped: ...
def peak_prominences(x: Untyped, peaks: Untyped, wlen: Untyped | None = None) -> Untyped: ...
_Array_n: TypeAlias = npt.NDArray[np.intp]
_Array_n_1d: TypeAlias = np.ndarray[tuple[int], np.dtype[np.intp]]
_Array_f8: TypeAlias = npt.NDArray[np.float64]
_Mode: TypeAlias = Literal["clip", "wrap"]

_ProminencesResult: TypeAlias = tuple[_Array_f8, _Array_n, _Array_n]
_WidthsResult: TypeAlias = tuple[_Array_f8, _Array_f8, _Array_f8, _Array_f8]

class _FindPeaksResultsDict(TypedDict, total=False):
peak_heights: _Array_f8
left_thresholds: _Array_f8
right_thresholds: _Array_f8
prominences: _Array_f8
left_bases: _Array_n
right_bases: _Array_n
width_heights: _Array_f8
left_ips: _Array_f8
right_ips: _Array_f8
plateau_sizes: _Array_n
left_edges: _Array_n
right_edges: _Array_n

def argrelmin(
data: npt.NDArray[np.generic],
axis: op.CanIndex = 0,
order: int = 1,
Copy link
Owner

Choose a reason for hiding this comment

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

I think that besides int, you could also pass some np.integer[Any] and np.bool_ to order. And coincidentally, scipy._typing.AnyInt is an alias for exactly that :)

There's also some other orders like this below

mode: _Mode = "clip",
) -> tuple[_Array_n, ...]: ...
def argrelmax(
data: npt.NDArray[np.generic],
axis: op.CanIndex = 0,
order: int = 1,
mode: _Mode = "clip",
) -> tuple[_Array_n, ...]: ...
def argrelextrema(
data: npt.NDArray[np.generic],
comparator: Callable[[npt.NDArray[np.generic], npt.NDArray[np.generic]], npt.NDArray[np.bool_]],
Copy link
Owner

Choose a reason for hiding this comment

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

totally optional; but you could use a TypeVar(..., bound=np.generic) instead of the current np.generics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that require the comparator function to have two arguments of the same type? I think that in the ideal case the function signature makes sense but what if someone were to pass a function that takes in a float and an int? They would still be able to do the comparison despite the types being different.

Assuming that using TypeVar enforces that the two argument types to be the same that is.

Copy link
Owner

@jorenham jorenham Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't that require the comparator function to have two arguments of the same type?

Yes exactly.

I think that in the ideal case the function signature makes sense but what if someone were to pass a function that takes in a float and an int?

If someone would pass a function that accepts arrays whose scalar types are incompatible, then it would indeed not be allowed.
And that's a good thing, because it could prevent some potentially type-unsafe situations.

Assuming that using TypeVar enforces that the two argument types to be the same that is.

Well, it kinda depends on what you mean with "the same".
Because in this case, we also need to take the relationship of NDArray and its scalar type parameter into account: It's covariant.
So that means that you're allowed to pass something like (NDArray[np.int32], NDArray[np.integer[Any]]) -> NDArray[np.bool_] as function if you also pass data: NDarray[np.int32].

For a demonstration of this, see https://basedpyright.com/?pythonVersion=3.13&typeCheckingMode=all&reportUnusedExpression=true&code=GYJw9gtgBAxmA28CmMAuBLMA7AzgOgEMAjGKdCABzBFSgGUkBHAVySxiQChRIpUBPCuiwBzMpWq0AwgUTFkAGlgEcqTpwDEULGFRI%2BACwK0CMOCAAmwsajB9BKAygDWSEDiUADYak9kcUCo46CJY8vq2UJ7A8GDGnuoCFPoAgiAgBPwA2gAqALpQALz0TKzsSLkFUFpwAG4EIOgEWLQAFADu1M44AJSa9slQaRnZ%2BUUlLGwclVDVZFj1jc1tFmBIOFgA5LSdIM59nBZIwFDAla0WxgQAXEPpmZVKcJQNxtS3MnJEyFlZww-5JT-UZ5PJA%2B7ZIhgBCgnpQAC0AD4oFCENdOLNZiAkKhmCAsIFEK1nhRXrYQBcrllrvCAIxgqCXVAELK0655HoHI4nHwAfWQvJicVQrRudxGWR8DKIt2BWSFxgZAHo4UjxQ9UfA8ujMVBsbj8VAsgR0FAADzFIim4DUQLoJRW%2BZQABe6AooodPTy6kOV3GMBUIrlUqU8HQqlaGVESFaAFZOX1w7ycJIkBZxsBKcylHyBQrUAcgA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got it. Just misunderstood the semantics. I can add this then.

axis: op.CanIndex = 0,
order: int = 1,
mode: _Mode = "clip",
) -> tuple[_Array_n, ...]: ...
def peak_prominences(
x: npt.ArrayLike,
peaks: _ArrayLikeInt_co,
wlen: AnyReal | None = None,
) -> _ProminencesResult: ...
def peak_widths(
x: Untyped,
peaks: Untyped,
rel_height: float = 0.5,
prominence_data: Untyped | None = None,
wlen: Untyped | None = None,
) -> Untyped: ...
x: npt.ArrayLike,
peaks: _ArrayLikeInt_co,
rel_height: AnyReal = 0.5,
prominence_data: _ProminencesResult | None = None,
wlen: AnyReal | None = None,
) -> _WidthsResult: ...
def find_peaks(
x: Untyped,
height: Untyped | None = None,
threshold: Untyped | None = None,
distance: Untyped | None = None,
prominence: Untyped | None = None,
width: Untyped | None = None,
wlen: Untyped | None = None,
rel_height: float = 0.5,
plateau_size: Untyped | None = None,
) -> Untyped: ...
x: npt.ArrayLike,
height: AnyReal | _Array_f8 | tuple[AnyReal | None, AnyReal | None] | None = None,
threshold: AnyReal | _Array_f8 | tuple[AnyReal | None, AnyReal | None] | None = None,
distance: AnyReal | None = None,
prominence: AnyReal | _Array_f8 | tuple[AnyReal | None, AnyReal | None] | None = None,
width: AnyReal | _Array_f8 | tuple[AnyReal | None, AnyReal | None] | None = None,
wlen: AnyReal | None = None,
rel_height: AnyReal = 0.5,
plateau_size: AnyInt | _Array_n | tuple[AnyInt | None, AnyInt | None] | None = None,
) -> tuple[_Array_n, _FindPeaksResultsDict]: ...
def find_peaks_cwt(
vector: Untyped,
widths: Untyped,
wavelet: Untyped | None = None,
max_distances: Untyped | None = None,
gap_thresh: Untyped | None = None,
min_length: Untyped | None = None,
min_snr: int = 1,
noise_perc: int = 10,
window_size: Untyped | None = None,
) -> Untyped: ...
vector: npt.ArrayLike,
widths: npt.ArrayLike,
wavelet: Callable[Concatenate[int, float, ...], npt.ArrayLike] | None = None,
Copy link
Owner

Choose a reason for hiding this comment

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

See my other comment on this signature

max_distances: Sequence[int] | None = None,
Copy link
Owner

Choose a reason for hiding this comment

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

I know it's a bit weird, but an ndarray isn't assignable to a Sequence, which the docs say that this should accept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thats a bit weird/annoying. The reason why I chose Sequence is because I didn't look at the docs for find_peaks_cwt (because it is a bit long) but instead for the function that max_distances gets passed to which is called _identify_ridge_lines which has says that max_distances is a 1-D sequence.

The source code for that function only calls len and indexes max_distances which fits the definition of Sequence. The values from max_distances gets compared to np.intp as well.

Would the type then be like onpt.Array[tuple[int], AnyInt]? That might still be too narrow considering that Sequence is a relatively weak type constraint.

Copy link
Owner

Choose a reason for hiding this comment

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

The problem with Sequence is that apart from __getitem__ it also has several other methods like .count() and .index() that aren't present in np.ndarray 🤷🏻

And yea I agree that onpt.Array (or some other direct np.ndarray alias) would be indeed too narrow. So you could use e.g. _ArrayLikeInt_co, which is compatible with both Sequence[int] and npt.NDArray[integer[Any]]. And as far as I'm concerned, the fact that it would also allow integer scalars isn't much of a problem, as we're talking about the input of a function.

gap_thresh: AnyInt | None = None,
Copy link
Owner

Choose a reason for hiding this comment

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

This could also be a float, according to the docs

min_length: AnyInt | None = None,
min_snr: AnyReal = 1,
noise_perc: AnyReal = 10,
window_size: AnyInt | None = None,
) -> _Array_n_1d: ...