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

feat: add dt.to_string to DaskExpr #796

Merged
merged 12 commits into from
Aug 19, 2024

Conversation

lucianosrp
Copy link
Member

@lucianosrp lucianosrp commented Aug 15, 2024

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.

Dask seems to be failing for %.f, any suggesstions on how to handle it ? Solved

@lucianosrp lucianosrp changed the title first test refactoring feat: add dt.to_string to DaskExpr Aug 15, 2024
@github-actions github-actions bot added the enhancement New feature or request label Aug 15, 2024
@lucianosrp lucianosrp marked this pull request as ready for review August 18, 2024 09:33
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.

That's definitly more effort than expected! Thanks for putting the work on this πŸ™ŒπŸΌ
I left only a nitpick comment on how to test/compare series values.

Edit: Just noticed the test failing, maybe a deeper look is needed πŸ₯²

tests/expr_and_series/dt/to_string_test.py Outdated Show resolved Hide resolved
tests/expr_and_series/dt/to_string_test.py Outdated Show resolved Hide resolved
Co-authored-by: Francesco Bruzzesi <[email protected]>
@lucianosrp
Copy link
Member Author

lucianosrp commented Aug 18, 2024

@FBruzzesi thanks as always, could you check now?
Edit: ah nevermind - still checking

@lucianosrp
Copy link
Member Author

I am not able to replicate this failure in my local, any suggestions ?

@FBruzzesi
Copy link
Member

I am not able to replicate this failure in my local, any suggestions ?

Sharing what I am discovering along the way: replacing with_columns with select does not raise an error. I am a bit lost as well πŸ™ˆ

What I mean exactly is:

result = nw.from_native(df).select(
    nw.col("a").dt.to_string("%Y-%m-%d")
)

@lucianosrp
Copy link
Member Author

replacing with_columns with select does not raise an error

Do you think there might be an issue with Narwhals' internals ? Or just Modin?
Shall we just change the test and monitor if we see this moving forward? πŸ˜“

@FBruzzesi
Copy link
Member

FBruzzesi commented Aug 19, 2024

I would not assume is something going wrong with narwhals since for pandas there are no issues and the logic should be the same. Apart from that, I never worked with modin directly πŸ₯²

@MarcoGorelli would you mind weighting in for this whenever you have the time?

@MarcoGorelli
Copy link
Member

thanks for the ping! will take a look at this and other open prs later today πŸ˜‡

@MarcoGorelli
Copy link
Member

looks like a bug in Modin - I've reported it to them here modin-project/modin#7371

shall we just xfail this test for modin for now?

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 @lucianosrp and @FBruzzesi for reviewing

I think you also need to xfail test_dt_to_string_iso_local_datetime_expr - then it should be good to go, feel free to merge once that's addressed (just check that all tests, apart from pyarrow-nightly, pass)

@lucianosrp
Copy link
Member Author

Thanks @FBruzzesi and @MarcoGorelli as always 🫑

@lucianosrp lucianosrp merged commit 676731c into narwhals-dev:main Aug 19, 2024
20 of 21 checks passed
@lucianosrp lucianosrp deleted the feat-dt-to_str branch August 19, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants