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

patch: stableify concat function #869

Merged
merged 5 commits into from
Aug 26, 2024
Merged

patch: stableify concat function #869

merged 5 commits into from
Aug 26, 2024

Conversation

FBruzzesi
Copy link
Member

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

See comment

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below.

@FBruzzesi FBruzzesi changed the title patch: stableify concat function patch: stableify concat function Aug 25, 2024
@FBruzzesi
Copy link
Member Author

I don't get how locally it passes. I am getting this some many times πŸ€” gotta be some config out of sync, but I am not sure what needs change

@MarcoGorelli
Copy link
Member

πŸ€” how annoying...nothing worse than a ci failure you can't reproduce...will take a look

@FBruzzesi
Copy link
Member Author

FBruzzesi commented Aug 25, 2024

πŸ€” how annoying...nothing worse than a ci failure you can't reproduce...will take a look

Thanks Marco! I was actually in a bit of a rush and can't put more effort before tomorrow.
The only thing I can think of is that the signature should be:

def concat(
+    items: Iterable[NwDataFrame[Any] | NwLazyFrame[Any]],
-    items: Iterable[DataFrame[Any] | LazyFrame[Any]],
    *,
    how: Literal["horizontal", "vertical"] = "vertical",
) -> DataFrame[Any] | LazyFrame[Any]:

since those stable dataframes are passed in. However then the "inner" nw.concat type hints would not comply.

Sorry for having to leave this hanging :(

@@ -24,7 +24,7 @@ def test_validate_laziness() -> None:
NotImplementedError,
match=("The items to concatenate should either all be eager, or all lazy"),
):
nw.concat([nw.from_native(df, eager_only=True), nw.from_native(df).lazy()])
nw.concat([nw.from_native(df, eager_only=True), nw.from_native(df).lazy()]) # type: ignore[list-item]
Copy link
Member Author

Choose a reason for hiding this comment

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

I am arguing that this is correct for mypy to complain, as it is supposed to raise an error and that's what we are checking

Copy link
Member

Choose a reason for hiding this comment

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

yup, and this is quite satisfying πŸ˜‹

Comment on lines +13 to +16
df_left = nw.from_native(constructor(data)).lazy()

data_right = {"c": [6, 12, -1], "d": [0, -4, 2]}
df_right = nw.from_native(constructor(data_right))
df_right = nw.from_native(constructor(data_right)).lazy()
Copy link
Member Author

Choose a reason for hiding this comment

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

Solution was to cast both to lazy so that mypy knows that it's not either eager or lazy.

Copy link
Member

Choose a reason for hiding this comment

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

ah I remember noticing this some time ago, then forgot about it...

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for doing this @FBruzzesi !

@FBruzzesi FBruzzesi merged commit 9dfd6f5 into main Aug 26, 2024
24 checks passed
@FBruzzesi FBruzzesi deleted the patch/stable-v1-concat branch August 26, 2024 17:38
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.

2 participants