addMapping() to support future multi-model plotting #7
Conversation
|
@curadomr Thanks for the PR! Overall this looks good. We'll need to remove the app files in I assume the GitHub Actions jobs aren't running because they are only triggered by a PR to "main", and this was originally opened against "dev". OmicNavigator/.github/workflows/comprehensive.yml Lines 20 to 22 in ca1cbf1 I'm going to close and re-open the PR in an attempt to trigger GitHub Actions 🤞 |
|
Alright, re-opening triggered the jobs. The good news is that it looks like all the failures are related to the files in |
|
The Windows and macOS jobs with R 4.1.2 passed. The Ubuntu job with R 3.4.4 failed to build the vignettes. It doesn't like this line from Error : vapply(mappingdf, is.character, logical(1)) are not all TRUE |
|
The vignette builds fine on my local Ubuntu with R 4.1.1. It's likely an issue with the older R version (3.4.4), as opposed to an issue with Ubuntu. |
- AddMapping accepts a list obj - checkMapping checks if mapping obj is a list, if entries are strings, and for overlapping features across models - validateMapping checks whether results features match with features present in mapping object and model names match model names from results table
|
|
R/check.R
Outdated
| listMaxLength <- max(sapply(mapping, length)) | ||
| mapping <- lapply(lapply(mapping, unlist), "length<-", listMaxLength) | ||
|
|
||
| mappingdf <- as.data.frame(mapping) |
There was a problem hiding this comment.
I figured out the error that only occurs with R 3.4.4. In R3, strings are always automatically coerced to factors by default (stringsAsFactors=TRUE). This default behavior was changed (for the better) in R4. Please update this line to explicitly keep the strings as charactor vectors when converting to a data frame.
mappingdf <- as.data.frame(mapping, stringsAsFactors = FALSE)
R/check.R
Outdated
| featAligned <- any(sum(!sapply(tempModel, is.na)) > 1) | ||
| } | ||
|
|
||
| if (isFALSE(featAligned)) { |
There was a problem hiding this comment.
Also, isFALSE() was added in (R 3.5.0](https://cran.r-project.org/doc/manuals/r-release/NEWS.3.html). Could you please change this to !featAligned? If you are worried about NAs, you can make it more robust with something like !is.na(featAligned) && !featAligned
There was a problem hiding this comment.
Just implemented in most recent commit. Thanks!
jdblischak
changed the title
Modifications to implement addMapping functionality, including tests and checks.
mapping object must be a list, with each element consisting in a vector of strings corresponding to the features found in a model, and named with the same name of the model.