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

docs: let nw.LazyFrame docstrings examples run on Polars and Dask only #1700

Conversation

AlessandroMiola
Copy link
Contributor

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

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

Related issues

Checklist

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

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

Runs nw.LazyFrame docstrings examples on Polars and Dask only.

@AlessandroMiola AlessandroMiola added the documentation Improvements or additions to documentation label Jan 1, 2025
Comment on lines 3528 to 3844
>>> agnostic_with_row_index(lf_dask).compute()
a b index
0 1 4 0
1 2 5 1
2 3 6 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we try to adjust it so that index comes first?

Copy link
Member

Choose a reason for hiding this comment

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

hmm well-spotted! i've opened an issue about it: #1703

Copy link
Member

Choose a reason for hiding this comment

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

Proposed fix in #1706

Copy link
Member

Choose a reason for hiding this comment

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

Let's just wait for #1706 to be merged first, then we can merge this one as well πŸ™ŒπŸΌ

@AlessandroMiola AlessandroMiola marked this pull request as ready for review January 1, 2025 19:03
@AlessandroMiola AlessandroMiola force-pushed the docs-lf-polars-dask-examples-only branch from d50b3e1 to e65d115 Compare January 1, 2025 19:29
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.

seriously awesome, thanks!

Comment on lines 3528 to 3844
>>> agnostic_with_row_index(lf_dask).compute()
a b index
0 1 4 0
1 2 5 1
2 3 6 2
Copy link
Member

Choose a reason for hiding this comment

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

hmm well-spotted! i've opened an issue about it: #1703

Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

Amazing work! Thanks a ton @AlessandroMiola

I wonder if we should .collect() within the agnostic functions so that we don't need to differentiate between compute and collect externally. Just a thought πŸ€” Any opinions on it?

@AlessandroMiola
Copy link
Contributor Author

AlessandroMiola commented Jan 2, 2025

Amazing work! Thanks a ton @AlessandroMiola

I wonder if we should .collect() within the agnostic functions so that we don't need to differentiate between compute and collect externally. Just a thought πŸ€” Any opinions on it?

Thanks @FBruzzesi!
Yeah, I agree! I guess ppl would probably go this way to fully exploit Narwhals if they have to materialize the frame? Moreover, I remember reading a similar comment somewhere in the Discord channel as well
I'll fix it

@AlessandroMiola AlessandroMiola force-pushed the docs-lf-polars-dask-examples-only branch from faac923 to 6674af0 Compare January 2, 2025 18:25
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.

seriously awesome, thanks @AlessandroMiola and @FBruzzesi for reviewing!

Comment on lines +455 to +467
>>> lf
β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”
| Narwhals LazyFrame |
|-----------------------------------|
|Dask DataFrame Structure: |
| a b c|
|npartitions=2 |
|0 string int64 int64|
|3 ... ... ...|
|5 ... ... ...|
|Dask Name: frompandas, 1 expression|
|Expr=df |
β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜
Copy link
Member

Choose a reason for hiding this comment

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

nice 😎

@MarcoGorelli MarcoGorelli merged commit 635ec46 into narwhals-dev:main Jan 4, 2025
24 checks passed
@AlessandroMiola AlessandroMiola deleted the docs-lf-polars-dask-examples-only branch January 4, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants