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

Better deprecation messages #9390

Closed
2 tasks done
thomasaarholt opened this issue Jun 16, 2023 · 2 comments
Closed
2 tasks done

Better deprecation messages #9390

thomasaarholt opened this issue Jun 16, 2023 · 2 comments
Labels
bug Something isn't working python Related to Python Polars

Comments

@thomasaarholt
Copy link
Contributor

Polars version checks

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of Polars.

Issue description

The API keeps changing, which is great (since its for the better!), but each change also adds some work to update code. I'm going to cheekily point out some challenges with #7784 (@MarcoGorelli, I'm just using yours because its the one I'm trying to fix in my code right now, nothing personal)

This message is a deprecation warning on the functionality of .pivot() from that PR:

In a future version of polars, the default `aggregate_function` will change from `'first'` to `None`. Please pass `'first'` to keep the current behaviour, or `None` to accept the new one.

Now, this particular case is a bit unusual because the new default is None, so instead we use a "NoDefault" as a temporary way to force the user to make a choice, which I agree with is a sensible decision.

My gripe is that:

  1. the deprecation message doesn't say what the new default argument does ('first' implies it chooses the first value, None doesn't really imply anything, but I would have assumed that it led to all cell values being list, as happens with groupby, e.g. df.groupby("foo").agg("bar") in the below example).
  2. I can't tell what None does, from the docstring or from the docstring code example:
import polars as pl
df = pl.DataFrame(
    {
        "foo": ["one", "one", "one", "two", "two", "two"],
        "bar": ["A", "B", "C", "A", "B", "C"],
        "baz": [1, 2, 3, 4, 5, 6],
    }
)
df1 = df.pivot(values="baz", index="foo", columns="bar", aggregate_function=None)
df2 = df.pivot(values="baz", index="foo", columns="bar", aggregate_function='first')
df1.frame_equal(df2) # True

So, in summary, I encourage us all to write verbose deprecation messages that rather give too much than too little information (these messages will be short-lived anyway, so they won't populate people's logs for long).

Reproducible example

.

Expected behavior

.

Installed versions

Replace this line with the output of pl.show_versions(), leave the backticks in place
@thomasaarholt thomasaarholt added bug Something isn't working python Related to Python Polars labels Jun 16, 2023
@MarcoGorelli
Copy link
Collaborator

thanks @thomasaarholt for bringing this up - yeah you're right, it's not clear what None does. did you want to make a PR to clarify? I think None just needs adding to the description of the aggregate_function parameter

@MarcoGorelli
Copy link
Collaborator

closed by #9473

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working python Related to Python Polars
Projects
None yet
Development

No branches or pull requests

2 participants