Move to a src layout and ship type information - #40
Merged
Conversation
The package was a single top-level module built from a .pyx at the repo root. That made it impossible to ship PEP 561 type information, since the py.typed marker has to live inside a package directory, and it let the test suite import from the working directory rather than from the installed extension. extinction is now a package under src/, with the compiled module as extinction._extinction and a thin __init__.py re-exporting the public API, so imports and __version__ are unchanged for callers. - Add py.typed and the _extinction.pyi stub, so mypy resolves return types and rejects a bad unit string or a non-float a_v. - Exclude the .pyx and generated .c from binary wheels. They now live inside the package directory, where MANIFEST.in would otherwise pull them into the wheel; the generated C alone is 1.4 MB, four times the size of the compiled module. They remain in the sdist. - Pass include_path to cythonize, since the include of extern/bsplines.pxi resolves against the project root rather than the directory holding the .pyx. Verified: sdist and wheel build, the sdist installs from source, the wheel contains only __init__.py, the .so, the stub and py.typed, the suite passes against the installed wheel from an unrelated directory, mypy resolves the stubs, and the docs still build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Moves extinction to a src package layout and adds PEP 561 typing support while preserving public imports.
Changes:
- Packages the compiled extension as
extinction._extinction. - Adds type stubs and a
py.typedmarker. - Updates build and distribution configuration.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/extinction/py.typed |
Marks the package as typed. |
src/extinction/_extinction.pyx |
Relocates the compiled implementation. |
src/extinction/_extinction.pyi |
Defines public type information. |
src/extinction/__init__.py |
Re-exports the public API. |
setup.py |
Builds the relocated extension. |
pyproject.toml |
Configures package discovery and package data. |
MANIFEST.in |
Includes package sources in source distributions. |
Suppressed comments (2)
src/extinction/_extinction.pyi:50
- This signature incorrectly restricts
applyto float64 arrays. Unlike the Cython wavelength APIs,applyis implemented with ordinary NumPy operations and valid calls with other floating dtypes (for example float32 extinction and flux arrays, including in-place use) are supported. The stub will reject those callers; use dtype-generic array types/overloads while preserving the in-place relationship toflux.
def apply(
extinction: _Array, flux: _Array, inplace: bool = ...
) -> _Array: ...
src/extinction/_extinction.pyi:53
removelikewise accepts NumPy floating arrays other than float64, but_Arrayrejects every such valid call. Please give this convenience API dtype-generic array types/overloads rather than reusing the float64-only alias required by the compiled wavelength functions.
def remove(
extinction: _Array, flux: _Array, inplace: bool = ...
) -> _Array: ...
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two mismatches with runtime behaviour, both verified against the built extension: - Fitzpatrick99.r_v is cdef readonly, so assigning to it raises AttributeError. A plain attribute annotation told type checkers the assignment was fine; it is now a read-only property. - apply() and remove() do plain numpy arithmetic rather than going through a typed memoryview, and accept a list, a float32 array or an int64 array. Annotating them as float64 arrays rejected valid calls. The wavelength arguments keep NDArray[np.float64]: those go through double[:] memoryviews and genuinely reject both lists (TypeError) and float32 arrays (ValueError). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The package was a single top-level module built from
extinction.pyxat the repo root. Two consequences: it was impossible to ship PEP 561 type information, because thepy.typedmarker has to live inside a package directory, and the test suite could import from the working directory instead of from the installed extension.extinctionis now a package undersrc/, with the compiled module asextinction._extinctionand a thin__init__.pyre-exporting the public API. Imports,__version__and the public API are unchanged for callers —import extinction; extinction.ccm89(...)works exactly as before.Type information
py.typedplus a stub for the compiled module. This adopts theextinction.pyithat was already sitting untracked in the working tree rather than a stub written from scratch — it is more precise than what I would have written, typingunitasLiteral["aa", "invum"]andwaveasNDArray[np.float64]rather than a looseArrayLike, which matches the implementation: thedouble[:]andnp.ndarrayparameters genuinely will not accept a plain list, and anyunitoutside those two raisesValueError.Checked with mypy against the installed wheel:
A wheel-bloat trap this introduced
Moving the sources inside the package directory meant
MANIFEST.instarted pulling them into the binary wheel — the generated_extinction.calone is 1.4 MB, roughly four times the size of the compiled.so. Wheels went from 104 KB to 296 KB before I caught it.[tool.setuptools.exclude-package-data]keeps the.pyxand.cin the sdist, where they belong, and out of wheels.Other mechanics
cythonizenow gets an explicitinclude_path, becauseinclude "extern/bsplines.pxi"in the.pyxresolves against the project root, not the directory holding the.pyx— without it the build breaks once the source moves.extern/deliberately stays at the repo root.Verification
--no-binary) and imports.__init__.py, the.so,_extinction.pyiandpy.typed— no sources.site-packages.Note for review
test.pystays at the repo root rather than moving totests/, to keep this diff to the layout change; the cibuildwheeltest-commandandMANIFEST.inreference it by path. Worth doing separately if you want it.🤖 Generated with Claude Code