pola-rs / r-polars

Polars R binding
https://pola-rs.github.io/r-polars/
Other
473 stars 35 forks source link

Open discussion on renaming polars classes #149

Closed sorhawell closed 11 months ago

sorhawell commented 1 year ago

I suggest to prefix all polars classes Polars, examples would be PolarsDataFrame PolarsSeries PolarsDataType

Pros:

Cons:

Bonus info

(**the long footnote) py-polars has 4 classes/structs from rust to python. DataFrame(rust-polars struct) -> PyDataFrame(wrapper struct) -> PyDataFrame (py03 internal class) -> DataFrame(py-polars public class). The vast majority of py-polars wrapper structs and wrapper classes are the thinnest possible. r-polars only has 3 classes/struct from rust to python: DataFrame(rust-polars struct) -> DataFrame(wrapper struct) -> DataFrame(extendr class). The R side DataFrame class though have both a set of public methods and a private method set. An early version of r-polars had 4 classes like py-polars and was using R6. That led to more conversions needed at runtime, more boiler plate code, and class instances was no longer just an R type ExternalPtr. Also, I felt it was sometimes cumbersome to make R6 behave exactly like py-polars syntax. Instead I decided to just implement extra base R S3 generic methods for the already auto-generated extendr-classes and add public method functions.

vincentarelbundock commented 1 year ago

FWIW, I agree that this is a useful change: I can imagine DataFrame vs. data.frame being confusing for some users.

I suggest to prefix all polars classes Polars, examples would be PolarsDataFrame PolarsSeries PolarsDataType

The names above seem fine. End-users will almost never need to write those out, so the length is almost irrelevant.

etiennebacher commented 1 year ago

To clarify, does this mean that

pl$DataFrame(a = 1:5, b = 1:5)

would change to

pl$PolarsDataFrame(a = 1:5, b = 1:5)

? Or that only the methods like ncol.DataFrame() are renamed to ncol.PolarsDataFrame()?

sorhawell commented 1 year ago

@etiennebacher hmm good question :/ I really just wanna have the cake and eat it to :)

long specfic class names, while short syntax. I hope it can make sense.

eitsupi commented 1 year ago

I feel that changes should be made to some of the following.

As for the function names in R, I think it makes sense to keep DataFrame, etc. since they are basically accessed by pl$.

sorhawell commented 1 year ago

Rust struct names like DataFrame, to something like RDataFrame (like PyDataFrame

I agree that is ideally best, but extendr does not currently support naming the R side and rust side differently. RDataFrame might look a bit strange on R side as the public class. On rust side it might not matter too much what the name is, just that it visually different from rust-polars class names e.g. DataFrame and Series. If we one day adopt S7 or extendr proc macros are updated, we can adopt RDataFrame and RSeries on rust side.

eitsupi commented 1 year ago

extendr does not currently support naming the R side and rust side differently.

It seems that extendr supports this. (r_name option)

#[extendr(
    r_name = "test.rename.rlike",
)]
fn test_rename() { }

https://github.com/extendr/extendr/blob/fc677380fa03c638e7913fdd96140da9ff1a5ad4/CHANGELOG.md?plain=1#L64-L75

Edit: I checked and found that r_name cannot actually be used to change the class name. I will try to update extendr.......

eitsupi commented 1 year ago

Related to extendr/extendr#553 and #253

eitsupi commented 1 year ago

Based on the discussion at #418, I think there is currently a consensus to adopt the prefix RPolars (e.g. RPolarsDataFrame).

etiennebacher commented 11 months ago

Where are we on this? #418 is closed so does that mean we can now rename all the classes RPolarsDataFrame, RPolarsExpr, etc. both on Rust and R sides?

eitsupi commented 11 months ago

Where are we on this? #418 is closed so does that mean we can now rename all the classes RPolarsDataFrame, RPolarsExpr, etc. both on Rust and R sides?

We simply need to rename the Rust structs here, since it seems unlikely that that functionality will be added to the extendr.