pola-rs / r-polars

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

fix: Keeps Enum order when converting to R #1252

Closed andyquinterom closed 1 month ago

andyquinterom commented 1 month ago

Closes #1251

etiennebacher commented 1 month ago

Hi, thanks for opening the issue and the corresponding PR, can you add a test and a bullet point in NEWS?

andyquinterom commented 1 month ago

Hi, thanks for opening the issue and the corresponding PR, can you add a test and a bullet point in NEWS?

I'll get on the tests asap

andyquinterom commented 1 month ago

@etiennebacher I added a test to validate many cases.

  # Generate 100 random data frames with random
  # orders for levels. They should always match
  for (i in 1:100) {
    expected_levels = sample(letters, length(letters))
    expected_values = sample(letters, length(letters))
    expect_identical(
      as_polars_series(expected_values)$
        cast(pl$Enum(expected_levels))$
        to_r(),
      factor(expected_values, levels = expected_levels)
    )
  }
andyquinterom commented 1 month ago

@etiennebacher Everything seems to pass except for R CMD check due to Non-api calls, but I think that's out of the scope of this pull request

etiennebacher commented 1 month ago

Everything seems to pass except for R CMD check due to Non-api calls, but I think that's out of the scope of this pull request

Yes this is out of scope. I agree with @eitsupi about using property-based testing on this. I think something like this would do the job (but I haven't tested yet):

test_that("reversing twice returns the original input", {
  for_all(
    values = any_atomic(any_na = TRUE),
    levels = any_atomic(any_na = TRUE),
    property = function(values, levels) {
        expect_identical(
          as_polars_series(values)$
            cast(pl$Enum(levels))$
            to_r(),
          factor(values, levels = levels)
        )
    }
  )
})

Please also add a bullet point in news with the PR number and your username (if you want).

andyquinterom commented 1 month ago

Thanks for the updates!

Could you bump the Rust lib version and please remove the lib-sums.tsv. Please check the document for details. https://github.com/pola-rs/r-polars/blob/8ecb1a358c5bf0c6c6e6c9527904bb798f07a101/DEVELOPMENT.md

I just ran the task and see that nothing has changed. Am I doing something wrong?

eitsupi commented 1 month ago

We should update lib version by hand first. https://github.com/pola-rs/r-polars/blob/main/src%2Frust%2FCargo.toml#L3-L3