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

DataFrame$tail() #103

Merged
merged 2 commits into from
Apr 12, 2023
Merged

DataFrame$tail() #103

merged 2 commits into from
Apr 12, 2023

Conversation

vincentarelbundock
Copy link
Collaborator

This is a first attempt at a minimal commit. Is this the kind of contribution you would welcome? What should I do different to facilitate a merge?

library(rpolars)

pl$DataFrame(mtcars)$tail(2)
# polars DataFrame: shape: (2, 11)
# ┌──────┬─────┬───────┬───────┬─────┬─────┬─────┬──────┬──────┐
# │ mpg  ┆ cyl ┆ disp  ┆ hp    ┆ ... ┆ vs  ┆ am  ┆ gear ┆ carb │
# │ ---  ┆ --- ┆ ---   ┆ ---   ┆     ┆ --- ┆ --- ┆ ---  ┆ ---  │
# │ f64  ┆ f64 ┆ f64   ┆ f64   ┆     ┆ f64 ┆ f64 ┆ f64  ┆ f64  │
# ╞══════╪═════╪═══════╪═══════╪═════╪═════╪═════╪══════╪══════╡
# │ 15.0 ┆ 8.0 ┆ 301.0 ┆ 335.0 ┆ ... ┆ 0.0 ┆ 1.0 ┆ 5.0  ┆ 8.0  │
# │ 21.4 ┆ 4.0 ┆ 121.0 ┆ 109.0 ┆ ... ┆ 1.0 ┆ 1.0 ┆ 4.0  ┆ 2.0  │
# └──────┴─────┴───────┴───────┴─────┴─────┴─────┴──────┴──────┘

tail(mtcars, 2)
#                mpg cyl disp  hp drat   wt qsec vs am gear carb
# Maserati Bora 15.0   8  301 335 3.54 3.57 14.6  0  1    5    8
# Volvo 142E    21.4   4  121 109 4.11 2.78 18.6  1  1    4    2

@sorhawell
Copy link
Collaborator

Hi @vincentarelbundock. Thank you so much for finding time for writing a translation :)

This is pretty alright. I would take the PR as it is right now.

I will add some suggestions in the review, I came to appreciate, when I wrote the later translations.

src/rust/src/lazy/dataframe.rs Show resolved Hide resolved
tests/testthat/test-dataframe.R Outdated Show resolved Hide resolved
tests/testthat/test-dataframe.R Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
@vincentarelbundock
Copy link
Collaborator Author

Thanks for the hints. I think the latest commit resolves all outstanding issues.

I still don't quite understand what is going on under the hood, but I'll try to translate a few other functions. Will open a separate PR for those.

@sorhawell
Copy link
Collaborator

Thanks for the hints. I think the latest commit resolves all outstanding issues.

I still don't quite understand what is going on under the hood, but I'll try to translate a few other functions.

Error handling in rpolars:
It is not possible to raise R errors from rust side with extendr without bleeding memory. Instead an Result-enum Result<LazyFrame, String> is returned to R side as an R list list(ok=NULL, err = "oups!"). On R side typically the Result is just unwrap()'ed immediately to raise the error.
r_result_list() is the old less ergonomic way to convert Result to R list. Now rpolars use a PR branch of extendr where the conversion to R list will occur implicitly, as you have done here.

robj_to!:
robj_to! is just a macro which do custom conversions from R to rust types + error handling. The vanilla extendr conversions aren't bad, but not always ideal and error messages are very sparse. By using robj_to! all conversions and err handling can be modified throughout the code base at one single place.
robj_to! basically dispatches the appropriate conversion function and do error handling. There is a candidate master student project to upgrade the error handling this summer.

@sorhawell
Copy link
Collaborator

many thanks @vincentarelbundock for this excellent contribution :)

@sorhawell sorhawell merged commit 5657a87 into pola-rs:main Apr 12, 2023
@vincentarelbundock
Copy link
Collaborator Author

Thanks for the merge and the info, this is very useful.

vincentarelbundock added a commit to vincentarelbundock/r-polars that referenced this pull request Apr 13, 2023
* DataFrame$tail()

* DataFrame_tail PR review
sorhawell added a commit that referenced this pull request Apr 14, 2023
* DataFrame translations: No arguments

* translate: DataFrame$reverse()

* translation: DataFrame$slice()

* typo in readme (#102)

* DataFrame$tail() (#103)

* DataFrame$tail()

* DataFrame_tail PR review

* merge conflict cruft

* GroupBy translations

* DataFrame$slice() optional arg

* translation DataFrame.var() and .std()

* DataFrame: null_count(), estimated_size()

* docs

* try to fix examples

* patrick

* patrick 2

* patrick 3

* patrick 4

* simplify basic LazyFrame methods

* roxydocs for previous commit

* add newline eof

* simplify DataFrame .null_count .estimated_size

* test .estimated_size()

* NEWS

* Revert "NEWS"

This reverts commit 76b6964.

* NEWS 2

* Revert "NEWS 2"

This reverts commit 6c11a63.

* test .null_count() on DataFrame and GroupBy

---------

Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
Co-authored-by: sorhawell <sorhawell@gmail.com>
@vincentarelbundock vincentarelbundock deleted the DataFrame_Tail branch April 23, 2023 12:24
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.

2 participants