🏷️ sparse: improved init stubs of sparse arrays with shape as positional argument.

[JulVandenBroeck]

Hello!

I noticed some typing errors in my project when creating empty sparse arrays.
This happens if the main argument (arg1) is a shape-like object and the dtype kwarg is not np.float64, leading to pyright thinking that arg1 is an array, instead of a shape.
I fixed this by adding more overloads for the constructor of all sparse array classes. I also added tests for these changes in the existing test file for CSR arrays.

Thanks for the great work on the stubs so far.
Hopefully, this change will benefit the project!

Kind regards,
Jul

PS: This is my first pull request, so sorry for any mistakes!

[jorenham]

Thanks for this PR! It's a pretty difficult problem to solve, but for the most part you have actually managed to do so, which tells me that you're not new to this :)

I left some comments; hopefully they make sense to you.

Is there any particular reason for why you didn't cover dok_array, or was that an oversight?

[jorenham]

This overload could be merged with the one at line 134:

Suggested change

	dtype: None = None,
	dtype: onp.AnyFloat64DType | None = None,

I believe that the same can be said for the other sparse formats.

[JulVandenBroeck]

So, I included a default overload with just the None option to cover the base case (where the dtype is np.float64). If I remove and merge it with the specific np.float64 overload, another overload (np.bool_ in this case) gets selected first when I do not specify a dtype.
Would you rather I put the np.float64 one at the top of the overloads, so that it gets selected first, or I keep the current overloads?

[JulVandenBroeck]

Ah, I think you are alluding to the first option in the suggestions below, so I'll stick to that one!

[jorenham]

Suggested change

	@overload # 2-d shape-like, dtype: type[float]
	def __init__(
	self: bsr_array[np.float64],
	/,
	arg1: ToShape2D,
	shape: ToShape2D | None = None,
	dtype: onp.AnyFloat64DType | None = None,
	copy: bool = False,
	*,
	maxprint: int | None = None,
	) -> None: ...

[jorenham]

Passing dtype=None won't result in a bsr_array[np.bool_], so the dtype shouldn't have a default here.

But because dtype can be either a positional- or a keyword-arg, and because the preceeding shape parameter has a default, we now have two separate situations we need to deal with:

1. bsr_array((2, 3), dtype=bool)
2. bsr_array((2, 3), None, bool)

The first form is probably what most users will go with in practice. But technically speaking, the second form is also valid, although I don't think it's very a likely choice. But to be honest, I have no numbers to back up that claim.

Personally I wouldn't mind if we only cover the first form here. But if you want to be complete about it and don't mind the additional complexity, then you could also add extra overloads for the second form.

This is how I'd annotate the first form:

Suggested change

	@overload # 2-d shape-like, dtype: type[bool]
	def __init__(
	self: bsr_array[np.bool_],
	/,
	arg1: ToShape2D,
	shape: ToShape2D | None = None,
	dtype: onp.AnyBoolDType | None = None,
	copy: bool = False,
	*,
	maxprint: int | None = None,
	) -> None: ...
	@overload # 2-d shape-like, dtype: bool-like (keyword)
	def __init__(
	self: bsr_array[np.bool_],
	/,
	arg1: ToShape2D,
	shape: ToShape2D | None = None,
	*,
	dtype: onp.AnyBoolDType,
	copy: bool = False,
	maxprint: int | None = None,
	) -> None: ...

and if we also cover the second form, that'd be

Suggested change

	@overload # 2-d shape-like, dtype: type[bool]
	def __init__(
	self: bsr_array[np.bool_],
	/,
	arg1: ToShape2D,
	shape: ToShape2D | None = None,
	dtype: onp.AnyBoolDType | None = None,
	copy: bool = False,
	*,
	maxprint: int | None = None,
	) -> None: ...
	@overload # 2-d shape-like, dtype: bool-like (keyword)
	def __init__(
	self: bsr_array[np.bool_],
	/,
	arg1: ToShape2D,
	shape: ToShape2D | None = None,
	*,
	dtype: onp.AnyBoolDType,
	copy: bool = False,
	maxprint: int | None = None,
	) -> None: ...
	@overload # 2-d shape-like, dtype: bool-like (positional)
	def __init__(
	self: bsr_array[np.bool_],
	/,
	arg1: ToShape2D,
	shape: ToShape2D | None,
	dtype: onp.AnyBoolDType,
	copy: bool = False,
	maxprint: int | None = None,
	) -> None: ...

I'm fine with either option, as long as you consistently apply it for all 3 dtypes, 7 sparse formats, and >=1 shape types (so >21 overloads).

[JulVandenBroeck]

I understand that the | None option shouldn't be there for the types other than np.float64, but in practice these overloads will never be picked, since now the np.float64 one is at the top and will be opted for first.
At the same time, the current implementation (with | None for each dtype) covers all cases.
So, I think it's at most a bit "hacky" to put | None after each dtype, but it does the job and avoids 21+ extra overloads.

What do you think? Could be I missed something :)

[jorenham]

but in practice these overloads will never be picked,

that is, unless you pass a dtype that is actually annotated as e.g. type[bool] | None :)

def create_sparse_array(dtype: bool | None = None):
    return scipy.sparse.bsr_array((2, 2), dtype=dtype)

[JulVandenBroeck]

Aha, I see.. I'll look into the overloads to resolve this then.

[JulVandenBroeck]

Love the fast and thorough response! I'm gonna go over the suggestions ASAP.

I indeed missed the DOK format, I'll add the overloads there as well.

For the follow-up, do I just push the new changes to my fork?
In other words: do I have to "re-PR" the new commit, or does this thread get updated automatically when I update my branch?

Edit: seems I guessed correctly that it gets updated automatically! I indeed have some knowledge about Python typing and GitHub, but this PR stuff is new to me. Seems to be working great though 😆

[JulVandenBroeck]

As you can see in the latest commit, I included test cases for the different signatures (with keywords and positional arguments) and they all pass with the current layout.

[jorenham]

For the follow-up, do I just push the new changes to my fork?
In other words: do I have to "re-PR" the new commit, or does this thread get updated automatically when I update my branch?

don't worry about the commits, we can just squash-merge them :)

[jorenham]

apart from the remaining dtype: _ | None = None stuff, this looks good to me

[JulVandenBroeck]

I implemented the 21+ overloads, did some tests, and believe it's correct now. :)

[jorenham]

Looks good!

One nitpick: the comments such as dtype: type[bool], only describe part of the story, because the onp.AnyBoolDType union type alias also includes types like Literal["?"].
So how about instead of type[bool], we refer to it as something intentionally vague, such as "bool-like"?

It shouldn't be much work if you use a (regex) search+replace or sed -e or something. But I also wouldn't mind if you don't feel like it, as this is already a great improvement right now :)

[JulVandenBroeck]

Sure! I've been using regex for the overloads, so might as well :D

[jorenham]

Thanks Jul!