Skip to content

Fix class error@main#808

Merged
BFalquet merged 4 commits intomainfrom
fix-class-error@main
Jan 23, 2025
Merged

Fix class error@main#808
BFalquet merged 4 commits intomainfrom
fix-class-error@main

Conversation

@BFalquet
Copy link
Copy Markdown
Contributor

bug spotted by @barnett11 in lbt05

The error I see is below,
Error in `arrange()`:
! Can't transform a data frame with `NA` or `""` names.
Run `rlang::last_trace()` to see where the error occurred.

I believe I've located issue at this section specifically within the main function,
map <- expand.grid(PARAM = levels(adam_db$adlb$PARAM), ABN_DIR = c("Low", 
        "High"), stringsAsFactors = FALSE) %>% arrange(.data$PARAM, 
        desc(.data$ABN_DIR))

This is due to the fact the PARAM is expected to be a factor by the level function. Automatic conversion to factor can cause unexpected behavior and is, in general, avoided.

Defensive approach is to modify the assertion to force PARAM to be a factor.

  • adapt assertion
  • add tests

thank you for the review

@BFalquet BFalquet added bug Something isn't working chevron labels Jan 22, 2025
@BFalquet BFalquet requested a review from clarkliming as a code owner January 22, 2025 16:25
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 22, 2025

Unit Tests Summary

  1 files   58 suites   3m 23s ⏱️
293 tests 292 ✅ 1 💤 0 ❌
687 runs  678 ✅ 9 💤 0 ❌

Results for commit b80ab97.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 22, 2025

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
lbt05 👶 $+0.71$ lbt05_works_with_missing_levels

Results for commit 1a723c1

♻️ This comment has been updated with latest results.

Comment thread tests/testthat/_snaps/lbt05.md
@BFalquet BFalquet requested a review from shajoezhu January 23, 2025 09:16
Copy link
Copy Markdown
Contributor

@shajoezhu shajoezhu left a comment

Choose a reason for hiding this comment

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

lgtm! thanks to Tim's explaination

@BFalquet BFalquet merged commit ec05d5d into main Jan 23, 2025
@BFalquet BFalquet deleted the fix-class-error@main branch January 23, 2025 13:44
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working chevron

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants