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

refactor!: rename <Expr>$lit_to_s() to <Expr>$to_series() and remove <Expr>$lit_to_df() #582

Merged
merged 12 commits into from
Dec 15, 2023

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Dec 8, 2023

  • These functions do not exist in py-polars
  • Their names differ from the rules for other functions (should be $to_* instead of $lit_to_*)
  • It is ambiguous whether "df" represents data.frame or RPolarsDataFrame

So I think it is better to remove them.

Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for me but I'd like @sorhawell to take a look too

@sorhawell
Copy link
Collaborator

sorhawell commented Dec 9, 2023

I disagree with dropping lit_to_s().

How about keeping lit_to_s() or renaming to to_s()?

I find it very practical and is one of my favorite methods. All the tests become more verbose by using as_polars_series() and it is less easy to fast tab-autocomplete and it breaks the polars syntax if required to use |> for conversion.

These functions do not exist in py-polars

Neither do as_polars_series() right?

Their names differ from the rules for other functions (should be $to_* instead of $lit_to_*)

That is because only the subvarient Expr::Literal or (Expr derived thereof) can be converted into a Series. I prefixed lit_ point this out.

#example of non literal conversion to Series
> as_polars_series(pl$col())

Error: Execution halted with the following contexts
0: In R: in $select()
0: During function call [as_polars_series(pl$col())]
1: Encountered the following error in Rust-Polars:
not found:

Error originated just after this operation:
DF []; PROJECT */0 COLUMNS; SELECTION: "None"

We can drop the prefix lit, but documentation should state that it will cause an error converting non Literal-derived expressions to Series. We can consider change to to_s().

It is ambiguous whether "df" represents data.frame or RPolarsDataFrame

agreed but that could be resolved by renaming to lit_to_xyz. I don't think lit_to_df is as important as lit_to_s().

@sorhawell
Copy link
Collaborator

sorhawell commented Dec 9, 2023

a use case for lit_to_s(): If working with a Series, we have no cast. to_lit() and lit_to_s() allows jumping back and forth with method chaining only.

s = pl$Series[1:3]
s$
  to_lit[]$
  # work with a Series as a Literal
  cast[pl$Float64]$
  lit_to_s[]$
  # back to Series interface
  append[4]$
  chunk_lengths[]
[1] 3 1

sry for hard brackets I got my self hooked tab-complete heaven :)

@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 10, 2023

I see.
Then how about deleting lit_to_df and renaming lit_to_s to to_series?

@sorhawell
Copy link
Collaborator

I see. Then how about deleting lit_to_df and renaming lit_to_s to to_series?

sounds good to my ears :)

@eitsupi eitsupi changed the title refactor!: remove <Expr>$lit_to_s() and <Expr>$lit_to_df() in favor of as_polars_* refactor!: rename <Expr>$lit_to_s() to <Expr>$to_series() and remove <Expr>$lit_to_df() Dec 15, 2023
@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 15, 2023

@sorhawell I have updated. Could you take a look at this?

Copy link
Collaborator

@sorhawell sorhawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me :)

@eitsupi eitsupi merged commit c74a2b8 into main Dec 15, 2023
17 checks passed
@eitsupi eitsupi deleted the remove-lit-to-s branch December 15, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants