RS-19848: Fix error due to stricter checking in R 4.5.0 #20
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an error in the flipPlots package that occurs due to stricter argument checking introduced in R 4.5.0. The fix addresses an issue with the unlist() function call in the getNodeGroups() function that was using incorrect syntax.
- Fixed incorrect
unlist()syntax that fails under R 4.5.0's stricter checking - Added test coverage for the "Last variable" link color option
- Incremented package version to 1.3.9
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| R/sankeydiagram.R | Fixed unlist() function call to use proper syntax with c() |
| tests/testthat/test-sankeydiagram.R | Added test case for "Last variable" link color option |
| DESCRIPTION | Updated package version from 1.3.8 to 1.3.9 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| getNodeGroups <- function(type, links) | ||
| { | ||
| num.nodes <- length(unique(unlist(links$source, links$target))) | ||
| num.nodes <- length(unique(c(unlist(links$source), unlist(links$target)))) |
There was a problem hiding this comment.
The fix correctly addresses the R 4.5.0 compatibility issue. The original unlist(links$source, links$target) was invalid syntax - unlist() takes a single list argument, not multiple arguments. Using c(unlist(links$source), unlist(links$target)) properly concatenates the unlisted vectors before finding unique values.
There was a problem hiding this comment.
Was it broken before? Does it fix a bug that the user would have noticed?
There was a problem hiding this comment.
In R 4.4.2 (which we currently use), there's no error but when we migrate to R 4.5.0 an error will be thrown when the user uses the link.color = "Last variable"
There was a problem hiding this comment.
Was it working before, i.e. links$target was used? The fix looks fine to me, just curious that it wasn't noticed as a bug.
There was a problem hiding this comment.
So num.nodes is being used to initialize the vector, but R is flexible with vector sizing, i.e. you can index beyond the vector length, so there was no bug I think.
There was a problem hiding this comment.
Yep, I reran the Standard R tests, and there are no diffs so I guess its not that important that num.nodes is set correctly, but it does make debugging a bit confusing and is probably a bit more inefficient
| test_that("Many categories", | ||
| { | ||
| expect_error(SankeyDiagram(datText), NA) | ||
| expect_error(SankeyDiagram(datText, link.color = "Last variable"), NA) |
There was a problem hiding this comment.
Is it not clear to me how this tests the bug (I'm not familiar with the code)
There was a problem hiding this comment.
If you look at the CI test results (which are using R 4.5.0), the test fails on the first commit. In terms of the code, the error is in getNodeGroups() which is called at line 132 of sankeydiagram.R and is only called when the link.color is "Last variable" or "First variable"
No description provided.