DM-56017: Add Python type annotations to sphgeom - #114
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
- Coverage 72.39% 72.33% -0.06%
==========================================
Files 116 116
Lines 7179 7180 +1
Branches 2291 2294 +3
==========================================
- Hits 5197 5194 -3
- Misses 412 413 +1
- Partials 1570 1573 +3 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
🟡 Changes recommended
The new stubs reference numpy.bool (breaking mypy with modern NumPy stubs) and the packaging config likely omits shipping .pyi files in wheels, undermining the PR’s typing goals.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds first-class static typing support to the lsst.sphgeom Python package (PEP 561), including a comprehensive __init__.pyi stub file and CI mypy checking, while tightening/aligning a few Python/pybind11 signatures to support that typing work.
Changes:
- Add bundled typing artifacts (
py.typed,__init__.pyi) and a mypy configuration + GitHub Action. - Refine/annotate several Python APIs (notably pixelization interfaces and HealpixPixelization) to be type-checker-friendly.
- Add explicit return types for several pybind11
__reduce__lambdas and fixLonLat’s__ne__binding name.
File summaries
| File | Description |
|---|---|
types.txt |
Declares typing-related dependency constraints (NumPy). |
python/lsst/sphgeom/py.typed |
Marks the package as typed (PEP 561). |
python/lsst/sphgeom/pixelization_abc.py |
Tightens abstract interface typing for pixelizations. |
python/lsst/sphgeom/_vector3d.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/_rangeSet.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/_q3cPixelization.cc |
Adds <pybind11/stl.h> and explicit __reduce__ return type. |
python/lsst/sphgeom/_normalizedAngle.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/_mq3cPixelization.cc |
Adds <pybind11/stl.h> and explicit __reduce__ return type. |
python/lsst/sphgeom/_matrix3d.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/_lonLat.cc |
Fixes __ne__ binding name and adds explicit __reduce__ return type. |
python/lsst/sphgeom/_htmPixelization.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/_healpixPixelization.py |
Adds/clarifies Python typing and improves equality semantics. |
python/lsst/sphgeom/_continue_class.py |
Adds typing aids/ignores to satisfy static checking while extending bound classes. |
python/lsst/sphgeom/_chunker.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/_box3d.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/_angle.cc |
Adds explicit pybind11 return typing for __reduce__. |
python/lsst/sphgeom/__init__.pyi |
Adds a large top-level stub file describing the public Python API. |
pyproject.toml |
Adds setuptools package-data entry for typing marker inclusion. |
mypy.ini |
Adds repo mypy configuration. |
include/lsst/sphgeom/python/interval.h |
Adds explicit pybind11 return typing for templated __reduce__. |
.gitignore |
Ignores an additional compiled test binary. |
.github/workflows/mypy.yaml |
Adds mypy CI via a reusable workflow. |
.github/pull_request_template.md |
Reminds contributors to update stubs when Python APIs change. |
Review details
Suppressed comments (10)
python/lsst/sphgeom/init.pyi:220
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:328
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:435
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:441
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:506
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:512
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:971
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:977
- This stub uses numpy.bool, which is not present in modern NumPy stubs (and is removed at runtime in newer NumPy versions); this will cause mypy to report "Module has no attribute 'bool'".
) -> typing.Annotated[numpy.typing.ArrayLike, numpy.bool]: ...
python/lsst/sphgeom/init.pyi:952
- RangeSet.ranges() is implemented to return a list of 2-tuples (first, last) (see python/lsst/sphgeom/_rangeSet.cc:ranges), but the stub uses an unparameterized list, losing type information.
def ranges(self) -> list: ...
python/lsst/sphgeom/init.pyi:910
- RangeSet.getitem returns a 2-tuple range (first, last) (python/lsst/sphgeom/_rangeSet.cc casts self.begin()[j], which is a pair-like range), so typing it as object (and iter as Iterator[object]) unnecessarily discards type information.
def __getitem__(self, arg0: int) -> object: ...
def __iter__(self) -> collections.abc.Iterator[object]: ...
- Files reviewed: 21/23 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c0fa5e9 to
5acb2d9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are mypy-impacting typing inconsistencies in the new annotations/config (notably __eq__ returning NotImplemented with a -> bool annotation, and a NumPy version mismatch between types.txt and pyproject.toml).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 21/23 changed files
- Comments generated: 2
- Review effort level: Lite
Stubs were generated with `stubgen` but needed a lot of manual fixes to make mypy happy.
5acb2d9 to
f13b66a
Compare
timj
left a comment
There was a problem hiding this comment.
Looks good. Hopefully at some point we can move some of these to the pybind11 code itself.
f13b66a to
8432cb6
Compare
8432cb6 to
d361097
Compare
Checklist
doc/changes