base.py - add type annoations

[idanmiara]

Addition type annotations to base.py as per #1687 (comment)
Discussion: #721
Related to #1687
Related to #1316

[coveralls]

Pull Request Test Coverage Report for Build 4048503165

• 87 of 88 (98.86%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.5%) to 87.212%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
shapely/geometry/base.py	75	76	98.68%

Totals
Change from base Build 4041902699:	0.5%
Covered Lines:	2421
Relevant Lines:	2776

---

💛 - Coveralls

[jorisvandenbossche]

A few inline comments, will post some general comments in the issue

[parched]

Would this be more precise?

Suggested change

	def distance(self, other: GeometryMaybeArrayLike) -> MaybeArray[float]:
	@typing.overload
	def distance(self, other: Geometry) -> float:
	...
	@typing.overload
	def distance(self, other: ArrayLike[Geometry]) -> Array[float]:
	...
	def distance(self, other: GeometryMaybeArrayLike) -> MaybeArray[float]:

[idanmiara]

I'm not familiar with this syntax, is def distance(self, other: GeometryMaybeArrayLike) -> MaybeArray[float]: needed if you add the first two ?
But still, there are 27 functions like this in the unit alone, I'm afraid that this might add up to a lot of overhead. WDYT ?

[parched]

Yes, you need that last one which has the actual implementation. Personally, I think it's worth the overhead, but I can understand if the Shapely maintainers say it isn't. In that case, just typing it as def distance(self, other: Geometry) -> float would be my preferred option, leaving the array case as an advanced option, not supported by the typing. How often is the array case used? We don't use it in our project, but unsure about most of Shapely's users.

[idanmiara]

How often is the array case used? We don't use it in our project, but unsure about most of Shapely's users.

Probably the more common use of the array inputs/outputs is in the top level functions which accept array also as the first geometry (instead of self) so I guess it makes more sense there.
In that case also I don't think that the generics solution would apply because there can be 2 ScalarOrArrayLike inputs (like shapely.intersection) and only if both are scalars than the output is scalar, and otherwise it's an array.
So for the top level functions for sure we need to type the arrays, but for base I don't have a strong opinion.

[caspervdw]

I think the object-oriented API of shapely is mostly used by people doing only scalar geometries. For these users, the typing doesn't really add value if you do (for instance) distance(...) -> MaybeArray[float]. A cast is then always necessary.

I am a bit torn on the solutions @parched suggested, but leaning towards the option of going all the way with the added overload. I'd like to hear the opinion of another maintainer on this @jorisvandenbossche @sgillies . Would you accept added @overload decorated methods for a 100% correct typing?

[idanmiara]

Would you accept added @overload decorated methods for a 100% correct typing?

Yes.

[caspervdw]

Thanks @idanmiara for this initiative.

As a generic remark; I am personally 👍 on the from __future__ import annotations, which would allow you to drop all the quotes around the types.

The discussion on whether to also type the Array option is a tough one, I'll jump in on that in the separate thread.

[idanmiara]

@caspervdw

As a generic remark; I am personally 👍 on the from __future__ import annotations, which would allow you to drop all the quotes around the types.

Forward references were deferred from python 3.11 because they might cause problems, see:
https://lukasz.langa.pl/61df599c-d9d8-4938-868b-36b67fdb4448/
Thus I'm 👎 for using from __future__ import annotations

[caspervdw]

@idanmiara I'd like to resume this subject of typing in shapely. But for this PR we still have a design issue which blocks implementation. Take for example the Geometry.contains. In this PR we have:

def contains(self, other: ScalarOrArrayLike["Geometry"]) -> ScalarOrArray[bool]:

Most people (@parched) work with scalar geometries. For these users, the problem here is that this will always need a cast(bool, result) to convince the typing that the return value is a bool. These users will actually find the following more useful:

def contains(self, other: "Geometry") -> bool:
    ...

Which, in my opinion, is also more comprehensible. However, the more correct solution (as stated before would be using overloads:

@typing.overload
def contains(self, other: "Geometry") -> bool:
    ...

@typing.overload
def contains(self, other: ArrayLike["Geometry"]) -> Array[bool]:
    ...

def contains(self, other: "Geometry" | ArrayLike["Geometry"]) -> bool | Array[bool]:
    ...  # actual code goes here

However, that is a lot of extra lines to cover and although it is 100% correct from a static type analysis point of view, it is harder to read.

We have to decide on this to go forward on typing. @idanmiara @jorisvandenbossche @sgillies could you give your opinion on this?

My personal perference is going with the scalar-only version (option 2) and postpone the arrays for later. Maybe we can experiment a bit with packing this in a can_also_accept_arrays decorator, or put this away into a stub file.

[idanmiara]

Hi @caspervdw
Thanks for your input!
I agree about the scalar only version for the Geometry methods.

I think that we might consider the overload version for the shapely top level functions. I would assume that users who use vectorized input would use these mostly and not the Geometry methods wrappers.

[sgillies]

@caspervdw I do not approve of how types are being figured out in a PR. I want to see an RFC or PEP type document explaining what the goal and scope of typing is and I want to see consensus on it before any work is considered. It feels to me like effort is being poured into this, and the effort is then being used to make a case for action. I don't think that's how we should proceed.

I think we'll have type hints eventually, but lack of them isn't an emergency. And they should be very carefully considered so they don't start this project accidentally onto a path of churning and breaking types as I've seen in other projects. pallets/click#2558 (comment) for example.

[idanmiara]

@caspervdw I do not approve of how types are being figured out in a PR. I want to see an RFC or PEP type document explaining what the goal and scope of typing is and I want to see consensus on it before any work is considered. It feels to me like effort is being poured into this, and the effort is then being used to make a case for action. I don't think that's how we should proceed.

@caspervdw what did you mean by saying the following?

We have to decide on this to go forward on typing.

Did the maintainers have an internal discussion about it already?
I don't mind writing an RFC if there is a consensus about including typing and we only need to decide the details. If you give me the the scope it could also help.

[mwtoews]

Shapely RFCs go to https://github.com/shapely/shapely-rfc and see potential inspiration from NumPy's NEP 41 and NEP 42.

[sgillies]

@mwtoews Thanks! I acknowledge that I'm raising the bar for working on type hints, and I probably owe the project some work on templating or reviewing the existing template.

Maybe, maybe, with such a document, a sponsor might be convinced to pay Idan to do such work, and pay others to review it. We've seen other sponsored Shapely work, and more would be fantastic.