r-polars icon indicating copy to clipboard operation
r-polars copied to clipboard

Change the arguments position of `pl$Series` with Python Polars

Open eitsupi opened this issue 1 year ago • 7 comments
trafficstars

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 avatar Mar 08 '24 15:03 eitsupi

@grantmcdermott Any thoughts on this?

eitsupi avatar Mar 10 '24 13:03 eitsupi

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.

grantmcdermott avatar Mar 10 '24 19:03 grantmcdermott

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 https://github.com/pola-rs/polars/issues/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 avatar Mar 10 '24 21:03 etiennebacher

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?

eitsupi avatar Mar 10 '24 21:03 eitsupi

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.

etiennebacher avatar Mar 10 '24 21:03 etiennebacher

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 avatar Mar 10 '24 22:03 eitsupi

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

eitsupi avatar Mar 25 '24 11:03 eitsupi