MAINT: type sph vor function signatures #11955
Conversation
tylerjereddy
added
scipy.spatial
maintenance
|
doc build fail is related, somewhat surprisingly |
|
@pmla @person142 thoughts? I haven't looked into this deeply yet, but hopefully an easy fix. |
tylerjereddy
force-pushed
the
sph_vor_type_hints_defs
branch
from
294e83b
to
a8468ab
Compare
|
I was able to reproduce the doc build error and fix locally using approach described by an In short, sphinx can't find Hopefully CI agrees.. |
scipy/spatial/ckdtree.pyi
Outdated
| class cKDTree: | ||
| def __init__(self, | ||
| data: np.ndarray, | ||
| leafsize: int = 16, |
There was a problem hiding this comment.
In stubs like this it's conventional to leave the actual default values out, so this would become leafsize: int = .... That prevents you from having to keep them in-sync with the real code.
There was a problem hiding this comment.
Ok, I've made this change for arguments in the header/stub file.
scipy/spatial/ckdtree.pyi
Outdated
| r: float, | ||
| p: float = 2., | ||
| eps: float = 0, | ||
| output_type: str = 'set') -> Union[set, np.ndarray]: ... |
There was a problem hiding this comment.
So with this you can get a more precise return type by using @overload and Literal. So that would look something like
from typing import overload, Set
from typing_extensions import Literal
@overload
def query_pairs(
....
) -> Set: ...
@overload
def query_pairs(
....
output_type: Literal['set']
) -> Set: ...
@overload
def query_pairs(
...
output_type: Literal['ndarray']
) -> np.ndarray: ...The numpy-stubs already require typing_extensions, so we're fine using them.
Generally unions in return types tend to be a pain (all calling code needs to be prepared to deal with all return types or use ignores, even when you really know what the return type is), so it's best to avoid them if at all possible.
There was a problem hiding this comment.
Here, I think I disagree--I tend to side with the confusion observed in this mypy issue:
python/mypy#5407
I spent ~20 minutes trying stuff and getting the same issue with Overloaded function signature 2 will never be matched.
This seems rather verbose and complex and mypy currently passes, so I'd rather not do that for now--maybe cleanup in the future?
* add type hints to `_spherical_voronoi.py` module function signatures * expansion of the `ckdtree.pyi` stub module with a few more call signatures was needed to allow type checking of the Cython calls without errors * sphinx can't find `Union[numpy.ndarray, List, None]` so we ignore this link using the approach descrbied by an astropy dev here: https://stackoverflow.com/a/30624034/2942522
* don't use default values in type hint header files to reduce maintenance burden
tylerjereddy
force-pushed
the
sph_vor_type_hints_defs
branch
from
a8468ab
to
652a8af
Compare
|
Just a question; might have been answered elsewhere: When does scipy use type annotations directly in the file and when a |
|
@MartinThoma in Python code it can be inline, in compiled code it can't. |
|
I'll let someone else drive this forward if it hasn't already been taken care of. A type hint PR that has been open for three years might as well be closed, and it is mine anyways so shouldn't annoy anyone. |
add type hints to
_spherical_voronoi.pymodulefunction signatures
expansion of the
ckdtree.pyistub module with a fewmore call signatures was needed to allow type checking of
the Cython calls without errors