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

api: "unstable" features #1367

Closed
MarcoGorelli opened this issue Nov 13, 2024 · 6 comments · Fixed by #1395
Closed

api: "unstable" features #1367

MarcoGorelli opened this issue Nov 13, 2024 · 6 comments · Fixed by #1395

Comments

@MarcoGorelli
Copy link
Member

I'm slightly hesitant about how to merge #1298 and #1290 - rolling_mean is marked as unstable in Polars. ewm_mean isn't, but follows the api kind of API style

I do think we should merge them, I just think we need a way to signal that we won't make the same kinds of promises for them as we do for the rest of the API in stable.v1

So, I'm tempted to say:

I'm sorry for the delay here in merging these PRs

@FBruzzesi
Copy link
Member

I'm sorry for the delay here in merging these PRs

No need to be sorry, I think #1290 was not even discussed fully (e.g. in which way we want to implement rolling for pyarrow backend).

Regarding how to mark features as unstable, which options do we have? Adding a warning/disclaimer in the docstrings and docs is the bare minimum, aside that:

  • Raising a warning might be too much and too strong for a user.
  • Raising a warning only in the stable API i.e. if used in narwhals.stable.v1, but let it go silently if called from narwhals?

I would say this second option could be a compromise?

@MarcoGorelli
Copy link
Member Author

thanks! the second one could work - we could define a NarwhalsUnstableWarning class, and advise users to silence warnings of that class if they're OK with using an experimental feature

@DeaMariaLeon
Copy link
Member

Is this decided then?: "define a NarwhalsUnstableWarning class, and advise users to silence warnings of that class if they're OK with using an experimental feature"

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Nov 15, 2024

Yeah I think this is probably the best way to go about it, thanks for the discussion

Are you interested in adding NarwhalsUnstableWarning to your PR (it could be defined in narwhals/_exceptions), or in adding it as a precursor PR?

EDIT: - Dea said on stream that she'd be happy to take this on, so I'll assign it to her

@DeaMariaLeon
Copy link
Member

So, the NarwhalsUnstableWarning is being done on #1395, I don't need to do this right @MarcoGorelli ?

@MarcoGorelli
Copy link
Member Author

i think you're right - i'll change the assignment then, once we get that in we can ship ewm_mean too 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants