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 DaskExpr.quantile #835

Merged
merged 4 commits into from
Aug 22, 2024
Merged

Conversation

anopsy
Copy link
Member

@anopsy anopsy commented Aug 21, 2024

Edit: Thanks for help @FBruzzesi

Hello, that's my attempt at solving Dask Expr.quantile- the method works as expected (according to my own tests).However I've been trying to find a way to include dask-case in our tests, since dask can work only with interpolation="nearest" and not the other ones.
What I tried:

  • adding is_dask param to the parameters in tests and running the tests using nullcontext, but compare dict doesn't want to cooperate with error messages,
  • writing separate test for dask cases and using if "dask" in str(contructor) to call that test but also failed here
    I was thinking about including every single constructor type in the parameters, but that seems a bit too verbose and the number of possible combinations is humongous.
    At the moment I'm exploring other options, but feel free to 🛟 send help 🛟 if you know how to approach it

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.

@FBruzzesi
Copy link
Member

Hey @anopsy thanks for the PR 🙌🏼

My 2 cents: I would edit test_quantile_expr test as:

- if "dask" in str(constructor):
+ if "dask" in str(constructor) and interpolation != "nearest":
    request.applymarker(pytest.mark.xfail)

@anopsy
Copy link
Member Author

anopsy commented Aug 21, 2024

I hope you enjoy your holiday 😃 Thank you so much I knew I'm missing some really simple solution 🫶

@FBruzzesi
Copy link
Member

FBruzzesi commented Aug 21, 2024

I think it is only missing an disclaimer/note in the exposed method and then it should be good to go?!

@anopsy anopsy marked this pull request as ready for review August 21, 2024 13:16
@anopsy anopsy changed the title dask-quantile-draft add: Dask Expr.quantile Aug 21, 2024
@anopsy
Copy link
Member Author

anopsy commented Aug 21, 2024

like that?
Note: dask has its own method to approximate quantile and it doesn't implement 'nearest', 'higher', 'lower', 'midpoint' as interpolation method - use 'linear' instead

@FBruzzesi
Copy link
Member

like that? Note: dask has its own method to approximate quantile and it doesn't implement 'nearest', 'higher', 'lower', 'midpoint' as interpolation method - use 'linear' instead

Yes, you could add it in the already existing note and create bullet points. If possible please consider adding an embedded link to the dask method as well.

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 @anopsy (and @FBruzzesi for reviewing!)

content looks good, merge when ready

@anopsy
Copy link
Member Author

anopsy commented Aug 21, 2024

@FBruzzesi cool, thanks for suggestion, was wondering how to arrange those two notes, also what do you mean by an embedded link to the dask method? A link to the dask docs?
Edit : this one? https://docs.dask.org/en/stable/generated/dask.dataframe.DataFrame.quantile.html

@FBruzzesi
Copy link
Member

We end up using Series.quantile

@FBruzzesi FBruzzesi changed the title add: Dask Expr.quantile feat: add DaskExpr.quantile Aug 21, 2024
@github-actions github-actions bot added the enhancement New feature or request label Aug 21, 2024
@MarcoGorelli MarcoGorelli merged commit 144f572 into narwhals-dev:main Aug 22, 2024
21 checks passed
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