Skip to content

fix: missing __getitem__ type fixes #1963

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.

Sign up for GitHub

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

Merged
MarcoGorelli merged 6 commits into from
Feb 8, 2025
Merged

fix: missing __getitem__ type fixes #1963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6272946
fix silly mistake
EdAbati Feb 7, 2025
89983fe
missing another np.array
EdAbati Feb 7, 2025
2517334
Merge branch 'main' into more-fixes-getitem
EdAbati Feb 8, 2025
88a0ca5
chore(typing): ignore type on fail case
dangotbanned Feb 8, 2025
932a2f9
fix(typing): add `int` cases to overloads
dangotbanned Feb 8, 2025
733ab52
chore(typing): ignore `pre-commit` false-positive
dangotbanned Feb 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions narwhals/dataframe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,16 +861,19 @@ def estimated_size(self: Self, unit: SizeUnit = "b") -> int | float:

@overload
def __getitem__( # type: ignore[overload-overlap]
self: Self, key: str | tuple[slice | Sequence[int] | np.ndarray, int | str]
self: Self,
item: str | tuple[slice | Sequence[int] | np.ndarray, int | str],
) -> Series[Any]: ...

@overload
def __getitem__(
self: Self,
key: (
slice
item: (
Copy link
Copy Markdown
Collaborator Author

@EdAbati EdAbati Feb 7, 2025

Choose a reason for hiding this comment

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

silly mistake (I blame copilot) 🙈

I wonder why it was not picked up though

int
Copy link
Copy Markdown
Collaborator Author

@EdAbati EdAbati Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @dangotbanned thanks for fixing! super quick I didn't have the chance to see the comments :D

Anyway I think that int is a bit of a special case.
The narwhals rule at the moment is that int in __getitem__ should not be fully supported (https://narwhals-dev.github.io/narwhals/pandas_like_concepts/column_names/) or encouraged .

I think we shouldn't add it in typing here, but ignore the int slicing in the tests if a typing error occurs.

Having said that, it is weird that after my changes mypy wants me to remove the # ignore in the tests

Copy link
Copy Markdown
Member

@dangotbanned dangotbanned Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@EdAbati interesting 🤔

So should this line in the doc be rewritten?

narwhals/narwhals/dataframe.py

Line 928 in 733ab52

- Integers are always interpreted as positions

I do appreciate that what you're saying is documented, but writing always reads to me as something that belongs in the annotation?

Copy link
Copy Markdown
Collaborator Author

@EdAbati EdAbati Feb 8, 2025

Choose a reason for hiding this comment

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

Mmm oh yeah actually good catch, you are right! then it makes a lot of sense

Copy link
Copy Markdown
Member

@dangotbanned dangotbanned Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@EdAbati marking as unresolved just to show another option if a reviewer wanted it:

diff --git a/narwhals/dataframe.py b/narwhals/dataframe.py
index 6b673a80..207ef7b2 100644
--- a/narwhals/dataframe.py
+++ b/narwhals/dataframe.py
@@ -859,6 +859,8 @@ class DataFrame(BaseFrame[DataFrameT]):
         """
         return self._compliant_frame.estimated_size(unit=unit)  # type: ignore[no-any-return]
 
+    @overload
+    def __getitem__(self: DataFrame[pd.DataFrame], item: int) -> Any: ...
     @overload
     def __getitem__(  # type: ignore[overload-overlap]
         self: Self,
diff --git a/narwhals/stable/v1/__init__.py b/narwhals/stable/v1/__init__.py
index 5aefefe2..af8aed47 100644
--- a/narwhals/stable/v1/__init__.py
+++ b/narwhals/stable/v1/__init__.py
@@ -84,6 +84,7 @@ if TYPE_CHECKING:
     from types import ModuleType
 
     import numpy as np
+    import pandas as pd
     from typing_extensions import Self
 
     from narwhals.dtypes import DType
@@ -131,6 +132,8 @@ class DataFrame(NwDataFrame[IntoDataFrameT]):
     def _lazyframe(self: Self) -> type[LazyFrame[Any]]:
         return LazyFrame
 
+    @overload
+    def __getitem__(self: DataFrame[pd.DataFrame], item: int) -> Any: ...
     @overload
     def __getitem__(  # type: ignore[overload-overlap]
         self: Self,

I think this describes the pandas edge case?

image

| slice
| Sequence[int]
| Sequence[str]
| np.ndarray
| tuple[
slice | Sequence[int] | np.ndarray, slice | Sequence[int] | Sequence[str]
]
Expand All @@ -880,9 +883,11 @@ def __getitem__(
self: Self,
item: (
str
| int
| slice
| Sequence[int]
| Sequence[str]
| np.ndarray
| tuple[slice | Sequence[int] | np.ndarray, int | str]
| tuple[
slice | Sequence[int] | np.ndarray, slice | Sequence[int] | Sequence[str]
Expand Down
9 changes: 6 additions & 3 deletions narwhals/stable/v1/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,16 @@ def _lazyframe(self: Self) -> type[LazyFrame[Any]]:

@overload
def __getitem__( # type: ignore[overload-overlap]
self: Self, key: str | tuple[slice | Sequence[int] | np.ndarray, int | str]
self: Self,
item: str | tuple[slice | Sequence[int] | np.ndarray, int | str],
) -> Series: ...
@overload
def __getitem__(
self: Self,
key: (
slice
item: (
int
| slice
| np.ndarray
| Sequence[int]
| Sequence[str]
| tuple[
Expand Down
2 changes: 1 addition & 1 deletion tests/frame/get_column_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@ def test_non_string_name() -> None:

def test_get_single_row() -> None:
df = pd.DataFrame({"a": [1, 2], "b": [3, 4]})
result = nw.from_native(df, eager_only=True)[0] # type: ignore[call-overload]
result = nw.from_native(df, eager_only=True)[0]
assert_equal_data(result, {"a": [1], "b": [3]})
4 changes: 2 additions & 2 deletions tests/frame/getitem_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ def test_slice_lazy_fails() -> None:


def test_slice_int(constructor_eager: ConstructorEager) -> None:
result = nw.from_native(constructor_eager(data), eager_only=True)[1] # type: ignore[call-overload]
result = nw.from_native(constructor_eager(data), eager_only=True)[1]
assert_equal_data(result, {"a": [2], "b": [12]})


def test_slice_fails(constructor_eager: ConstructorEager) -> None:
class Foo: ...

with pytest.raises(TypeError, match="Expected str or slice, got:"):
nw.from_native(constructor_eager(data), eager_only=True)[Foo()] # type: ignore[call-overload]
nw.from_native(constructor_eager(data), eager_only=True)[Foo()] # type: ignore[call-overload, unused-ignore]
Copy link
Copy Markdown
Member

@MarcoGorelli MarcoGorelli Feb 8, 2025

Choose a reason for hiding this comment

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

🤔 ignoring unused-ignore seems a bit strange?

Copy link
Copy Markdown
Member

@dangotbanned dangotbanned Feb 8, 2025

Choose a reason for hiding this comment

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

@MarcoGorelli see #1963 (comment)

It is to ignore pre-commit not understanding the call is invalid

Copy link
Copy Markdown
Member

@MarcoGorelli MarcoGorelli Feb 8, 2025

Choose a reason for hiding this comment

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

thanks!



def test_gather(constructor_eager: ConstructorEager) -> None:
Expand Down
Loading