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

ENH: allow tuple pad_width in pad #82

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Conversation

ev-br
Copy link
Contributor

@ev-br ev-br commented Jan 7, 2025

Observed in scipy/scipy#22226 : allow pad_width to be a tuple.

@ev-br ev-br marked this pull request as draft January 7, 2025 09:04
@ev-br
Copy link
Contributor Author

ev-br commented Jan 7, 2025

Testing scipy/scipy#22122 and scipy/scipy#22226 shows the need to bite the bullet and add pad_width being an int or a 2-tuple or a list of 2-tuples.
When round-tripping through numpy, it was all hidden out of sight; otherwise, need to implement this manually, as this PR does now.

@ev-br ev-br marked this pull request as ready for review January 7, 2025 09:39
@ev-br ev-br force-pushed the pad_tuples branch 2 times, most recently from a5ff0de to 2886728 Compare January 7, 2025 10:09
@ev-br ev-br force-pushed the pad_tuples branch 3 times, most recently from cbb96f2 to 6e7c94c Compare January 7, 2025 10:16
@lucascolley
Copy link
Collaborator

The remaining mypy failures can be ignored with inline comments, I think.

@ev-br
Copy link
Contributor Author

ev-br commented Jan 7, 2025

I really don't understand what on earth it wants and how to silence it. Now taht you want all this on, I'd appreciate if you could silence it with whatever means you prefer as appropriate.
(in my projects, I'd just shut all this off and be done with it; you clearly prefer some other way)

@lucascolley
Copy link
Collaborator

will do!

Comment on lines 598 to 609
if isinstance(pad_width, int):
pad_width = cast(list[tuple[int, int]], [(pad_width, pad_width)] * x.ndim)

if isinstance(pad_width, tuple):
pad_width = [pad_width] * x.ndim

if xp is None:
xp = array_namespace(x)

slices = []
newshape = []
for ax, w_tpl in enumerate(pad_width):
Copy link
Collaborator

@lucascolley lucascolley Jan 7, 2025

Choose a reason for hiding this comment

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

@DetachHead Basedpyright seems to be missing this assignment on line 599. With or without the explicit cast I've added here, we see:

/home/runner/work/array-api-extra/array-api-extra/src/array_api_extra/_funcs.py:609:32 - error: Argument of type "int | tuple[int, int] | list[tuple[int, int]]" cannot be assigned to parameter "iterable" of type "Iterable[_T@enumerate]" in function "__new__"
  Type "int | tuple[int, int] | list[tuple[int, int]]" is not assignable to type "Iterable[_T@enumerate]"
    "int" is incompatible with protocol "Iterable[_T@enumerate]"
      "__iter__" is not present (reportArgumentType)

https://github.com/data-apis/array-api-extra/actions/runs/12651016113/job/35250714798?pr=82

By line 609 pad_width must be a list. Is this a known issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

adding the three casts in

diff --git a/src/array_api_extra/_funcs.py b/src/array_api_extra/_funcs.py
index d5ee7fe..8fa397b 100644
--- a/src/array_api_extra/_funcs.py
+++ b/src/array_api_extra/_funcs.py
@@ -596,7 +596,7 @@ def pad(
 
     # make pad_width a list of length-2 tuples of ints
     if isinstance(pad_width, int):
-        pad_width = cast(list[tuple[int, int]], [(pad_width, pad_width)] * x.ndim)
+        pad_width = [(pad_width, pad_width)] * x.ndim
 
     if isinstance(pad_width, tuple):
         pad_width = [pad_width] * x.ndim
@@ -606,6 +606,7 @@ def pad(
 
     slices = []
     newshape = []
+    pad_width = cast(list[tuple[int, int]], pad_width)
     for ax, w_tpl in enumerate(pad_width):
         if len(w_tpl) != 2:
             msg = f"expect a 2-tuple (before, after), got {w_tpl}."
@@ -624,6 +625,8 @@ def pad(
         newshape.append(sh)
         slices.append(sl)
 
+    newshape = cast(list[int], newshape)
+    slices = cast(list[slice], slices)
     padded = xp.full(
         tuple(newshape),
         fill_value=value,
diff --git a/tests/test_funcs.py b/tests/test_funcs.py
index 4ae57ea..2fc9041 100644
--- a/tests/test_funcs.py
+++ b/tests/test_funcs.py
@@ -426,7 +426,7 @@ class TestPad:
         assert padded.shape == (6, 7)
 
         with pytest.raises(ValueError, match="expect a 2-tuple"):
-            pad(a, [(1, 2, 3)])  # type: ignore[list-item]
+            pad(a, [(1, 2, 3)])  # type: ignore[list-item]  # pyright: ignore[reportArgumentType]
 
     def test_list_of_tuples_width(self):
         a = xp.reshape(xp.arange(12), (3, 4))

fixes the basedpyright errors, but now basedmypy complains about a redundant cast:

src/array_api_extra/_funcs.py:609:17: error: Redundant cast to "list[(int, int)]"  [redundant-cast]
        pad_width = cast(list[tuple[int, int]], pad_width)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@KotlinIsland looks like there is something basedmypy is catching which basedpyright isn't? Any idea what?

Choose a reason for hiding this comment

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

it looks like pyright isn't narrowing it because x is an alias to Any. here's a minified repro

from typing import Any


def _(value: tuple[int, ...] | list[int] | int, x: Any):
    if isinstance(value, int):
        value = [value]

    reveal_type(value) # list[int] | tuple[int, ...]
    
    if isinstance(value, tuple):
        value = [1] * x
    
    reveal_type(value) # tuple[int, ...] | list[int] | int

i think this behavior is more correct than what mypy is doing, because the Any could have an __rmul__ that changes the type to something completely different. in fact i think this should probably widen value to Any instead because the type could be changed to anything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, thanks for the fast response! I'll add a cast and ignore the mypy error then 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

this playground has the entire function almost unmodified and it still doesn't reproduce the failure: https://mypy-play.net/?mypy=basedmypy-latest&python=3.13&gist=11d14b9d1c9bd562ef9b6554268e97f5

Choose a reason for hiding this comment

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

the error is that slice takes three generic parameters, i think the playground has an outdated version from prior to this.
ymyzk/mypy-playground#821

i'm not sure why it's reporting explicit any, i would expect it to report "Missing type parameters for generic type"

Copy link
Collaborator

@lucascolley lucascolley Jan 7, 2025

Choose a reason for hiding this comment

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

thanks, that makes sense why I can't reproduce on the playground. With the following diff:

diff --git a/src/array_api_extra/_funcs.py b/src/array_api_extra/_funcs.py
index 2f4bc0f..0c9f3e3 100644
--- a/src/array_api_extra/_funcs.py
+++ b/src/array_api_extra/_funcs.py
@@ -605,7 +605,7 @@ def pad(
         xp = array_namespace(x)
 
     # https://github.com/data-apis/array-api-extra/pull/82#discussion_r1905688819
-    slices: list[slice] = []  # type: ignore[no-any-explicit]
+    slices: list[slice[int | None, int | None, None]] = []
     newshape: list[int] = []
     for ax, w_tpl in enumerate(pad_width):
         if len(w_tpl) != 2:
@@ -613,6 +613,7 @@ def pad(
             raise ValueError(msg)
 
         sh = x.shape[ax]
+        sl: slice[int | None, int | None, None]
         if w_tpl[0] == 0 and w_tpl[1] == 0:
             sl = slice(None, None, None)
         else:

I now see:

src/array_api_extra/_funcs.py:618:18: error: Incompatible types in assignment (expression has type "slice[int | Any, int | Any, int | Any]", variable has type "slice[int | None, int | None, None]") 
[assignment]
                sl = slice(None, None, None)
                     ^~~~~~~~~~~~~~~~~~~~~~~
src/array_api_extra/_funcs.py:623:18: error: Incompatible types in assignment (expression has type "slice[int | Any, int | Any, int | Any]", variable has type "slice[int | None, int | None, None]") 
[assignment]
                sl = slice(start, stop, None)
                     ^~~~~~~~~~~~~~~~~~~~~~~~

can you see where the Any is coming from? Perhaps something isn't working with slice being generic over None?

Choose a reason for hiding this comment

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

def __new__(
        cls, start: int | None, stop: int | None, step: int | None = None, /
    ) -> slice[int | MaybeNone, int | MaybeNone, int | MaybeNone]: ...

where MaybeNone = Any

and the reason a: slice contains explicit Any is that:

_StartT = TypeVar("_StartT", covariant=True, default=Any)
_StopT = TypeVar("_StopT", covariant=True, default=Any)
_StepT = TypeVar("_StepT", covariant=True, default=Any)

class slice(Generic[_StartT, _StopT, _StepT]):

therefore:

a: slice
# is the same as
a: slice[Any, Any, Any]

so this is an issue with typeshed: python/typeshed#13376

Copy link
Collaborator

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

thanks @ev-br, merging!

do let me know if you have a clue what is going on with the failure I can't reproduce in the playground @KotlinIsland ! EDIT: just saw your reply

@lucascolley lucascolley merged commit 70c22c0 into data-apis:main Jan 7, 2025
10 checks passed
@lucascolley lucascolley added the enhancement New feature or request label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants