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.

Sign up for GitHub

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

Jump to bottom

Change the arguments position of pl$Series with Python Polars #903

Closed
eitsupi opened this issue Mar 8, 2024 · 7 comments · Fixed by #1071
Closed

Change the arguments position of pl$Series with Python Polars #903

eitsupi opened this issue Mar 8, 2024 · 7 comments · Fixed by #1071
Labels
enhancement New feature or request
Milestone
0.17

Comments

@eitsupi
Copy link
Collaborator

eitsupi commented Mar 8, 2024

In Python:

pl.Series("a", [1, 2, 3])

In R:

pl$Series(c(1, 2, 3), "a")

And the document says:

It is possible to construct a Series with values as the first positional argument. This syntax considered an anti-pattern

https://github.com/pola-rs/polars/blob/25d0a2f028bd2de0bdc140a2c040f2e2b4c2f46e/py-polars/polars/series/series.py#L225-L227

I suspect that as_polars_series should be used as a function that takes a vector as its first argument, and pl$Series should be rewritten in the argument position to match Python.
This would be a very huge breaking change, so we will probably have to continue warnings for several months and then make the change.

@etiennebacher Thoughts?
@eitsupi eitsupi added the enhancement New feature or request label Mar 8, 2024
@eitsupi eitsupi added this to the 0.16 milestone Mar 8, 2024
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 10, 2024

@grantmcdermott Any thoughts on this?

@grantmcdermott
Copy link
Collaborator

This would be a very huge breaking change, so we will probably have to continue warnings for several months and then make the change.

Oof, I think you're correct and there's probably no other way around it. Consistency across r-polars and py-polars has always been a (the?) North Star for this project, so this is just an unfortunate cost.

@etiennebacher
Copy link
Collaborator

If we do this, yes we should add a deprecation warning for a few releases before changing the behavior. However, I'd rather wait for pola-rs/polars#13405 to be settled before going forward with this. It seems like the order of arguments is unlikely to change but let's wait for a final decision

@etiennebacher etiennebacher changed the title The arguments posision of pl$Series should be change to match with Python Polars? The arguments position of pl$Series should be changed to match with Python Polars? Mar 10, 2024
@eitsupi eitsupi removed the upstream label Mar 10, 2024
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 10, 2024

Thanks both.

However, I'd rather wait for pola-rs/polars#13405 to be settled before going forward with this. It seems like the order of arguments is unlikely to change but let's wait for a final decision

If a change is made in Python, I think it can be reverted, but shouldn't it be fixed that the function argument is incorrect right now?

@etiennebacher
Copy link
Collaborator

I don't understand your point. Right now, they name - values and we have values - name. If they end up changing to values - name, then our implementation becomes correct, right? We must add a deprecation warning only if we're sure they stay with name - values.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 10, 2024 •
edited

I'm just suspicious that it needs to be changed because they're there right now.
At the very least, I think it's worth warning that positional is vulnerable, and always recommending named arguments. (By the way, the value argument doesn't exist in R right now, so we need to change that as well.)

@eitsupi eitsupi mentioned this issue Mar 18, 2024
Merged
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 25, 2024

pola-rs/polars#13405 has been marked as "not planed", so we will change the argument order.

@eitsupi eitsupi changed the title The arguments position of pl$Series should be changed to match with Python Polars? Change the arguments position of pl$Series with Python Polars Mar 25, 2024
@eitsupi eitsupi self-assigned this Mar 25, 2024
@eitsupi eitsupi mentioned this issue Mar 25, 2024
Merged
@eitsupi eitsupi modified the milestones: 0.16, 0.17 Mar 25, 2024
@eitsupi eitsupi removed their assignment Mar 25, 2024
@eitsupi eitsupi mentioned this issue May 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.17
Development

Successfully merging a pull request may close this issue.

fix!: change the argument order of pl$Series() to match to other languages pola-rs/r-polars
3 participants
@grantmcdermott @eitsupi @etiennebacher