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.describe + auto-convert String-Err to RPolarsErr #268

Merged
merged 18 commits into from
Jul 3, 2023

Conversation

sorhawell
Copy link
Collaborator

@sorhawell sorhawell commented Jun 27, 2023

Aim

use describe as occasion to rework RPolarsErr conversion behavior.

Before string errors and RPolarsErr where treated "equal". Now string errors will be upgraded to RPolarsErr. String errors can arise from legacy code with naive errors and from any base R function emitting errors.

Prior if an error was String it would propagate as that to the final error to the user.
Now R side result() and rust side unpack_r_result_list immediately upgrade errors to RPolarsErr.

RPolarsErr builds context in a more rigorous way and produces better traces of the error.

Summary of changes:

  • >DataFrame>$describe()
  • internal S3 method upgrade_err which can convert R errors and any legacy error into RPolarsErr.
  • now when result() captures errors they will be upgraded with upgrade_err()
  • .. also when R result_list unpacked any error will be upgraded to robj_to!
  • replace protoProxyArray with robj_to! in select
  • rework inner rust robj_to_rexpr being called by robj_to!
  • no long support $select(list_1, list_2) on multiple lists args. Either input is one single arg which is one Expression a list of expressions. OR input is multiple args which are expressions. (this matches py-polars behavior)

sorhawell and others added 10 commits June 28, 2023 15:14
Merge branch 'main' into impl_describe

# Conflicts:
#	tests/testthat/test-lazy.R
Merge branch 'main' into impl_describe

# Conflicts:
#	R/dataframe__frame.R
#	src/rust/src/utils/mod.rs
#	tests/testthat/_snaps/dataframe.md
#	tests/testthat/test-dataframe.R
Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Please auto formatting (make fmt GIT_DIF_TARGET=main)

@sorhawell
Copy link
Collaborator Author

make fmt GIT_DIF_TARGET=main

thx I misunderstood how to use make fmt

@eitsupi
Copy link
Collaborator

eitsupi commented Jul 3, 2023

thx I misunderstood how to use make fmt

Yes, currently by default only uncommitted files are formatted, so you need to set GIT_DIF_TARGET=main to format after a commit.......

@sorhawell
Copy link
Collaborator Author

hmm maybe bad merge, seems like I'm reverting reverse -> descending PR

@eitsupi
Copy link
Collaborator

eitsupi commented Jul 3, 2023

hmm maybe bad merge, seems like I'm reverting reverse -> descending PR

I think this is due to #291 (comment)

I will open a follow up PR...

@sorhawell sorhawell changed the title WIP: DataFrame.describe + auto-convert String-Err to RPolarsErr DataFrame.describe + auto-convert String-Err to RPolarsErr Jul 3, 2023
@sorhawell sorhawell marked this pull request as ready for review July 3, 2023 11:20
Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks!

@eitsupi eitsupi merged commit 11b1ca7 into main Jul 3, 2023
11 checks passed
@eitsupi eitsupi deleted the impl_describe branch July 3, 2023 14:48
@eitsupi eitsupi mentioned this pull request Jul 3, 2023
@ddotta
Copy link

ddotta commented Jul 26, 2023

Awesome !

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

4 participants