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.

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

shapely: Fix tuple length of CoordinateSequence items #13435

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

hamdanal
Copy link
Contributor

Closes #13422

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

How about making this a TypeVar? I can't find a way to encode this directly in the __init__ using the np.ndarray type (ie: extracting the shape size into a tuple length), but you could do this using TypeVar default:

_CoordinateTuple_co = TypeVar("_CoordinateTuple_co", covariant=True, bound=tuple[float, ...], default=tuple[float, ...])

class CoordinateSequence(Generic[_CoordinateTuple_co]):
    def __init__(self, coords: NDArray[np.float64]) -> None: ...
    def __len__(self) -> int: ...
    def __iter__(self) -> Iterator[_CoordinateTuple_co]: ...
    @overload
    def __getitem__(self, key: int) -> _CoordinateTuple_co: ...
    @overload
    def __getitem__(self, key: slice) -> list[_CoordinateTuple_co]: ...
    def __array__(self, dtype: DTypeLike | None = None, copy: Literal[True] | None = None) -> NDArray[np.float64]: ...
    @property
    def xy(self) -> tuple[array[float], array[float]]: ...

Which makes it possible to "opt-in" stricter/more accurate length-checks.

my_array: NDArray[np.float64] = ...
# revealed type: CoordinateSequence[tuple[float, ...]]
test = CoordinateSequence(my_array)
# revealed type: CoordinateSequence[tuple[float, float]]
test2 = CoordinateSequence[tuple[float, float]](my_array)
# revealed type: CoordinateSequence[tuple[float, float, float]]
test3 = CoordinateSequence[tuple[float, float, float]](my_array)

Otherwise looks good to me.

@hamdanal
Copy link
Contributor Author

hamdanal commented Feb 1, 2025

Thanks for the review Avasam.

CoordinateSequence is an internal class that should not be initialized directly by the user. Also, we don't currently keep track of whether shapely objects have a z coordinate or not anywhere in shapely, so it would be useless to have this class generic if we can never specialize it (also I use shapely extensively and never had a use case for it).
Using tuple[float, ...] works very well because both mypy and pyright will happily allow you to unpack it to x, y or x, y, z. Do you have a use case for what you proposed?

Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

Thank you for the additional details! I don't use shapely so my comment was entirely based on looking at the implementation. With your additional comments in mind, I think your PR is completely fine as-is.

If someone actually needs to make the distinction on the tuple size, then they can request it.

This is a very simple PR so I'll merge now. I don't expect pushback from other maintainers on this.

@Avasam Avasam merged commit d704a7d into python:main Feb 1, 2025
43 checks passed
@hamdanal hamdanal deleted the issue-13422 branch February 1, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shapely stubs for CoordinateSequence do not cover 2.5D case
2 participants