Updating plots.R for multi_mapping #12
Conversation
curadomr
commented
Sep 15, 2022
curadomr
commented
- if one single mapping is provided, it should be called 'defaults'
- if > 1 mapping object is provided, it should be named after a model name (as provided in addModels()). For such cases, when having modelID as a vector of models in plotStudy call (or getPlottingData), modelID[1] is picked as the name for the mapping object.
- if one single mapping is provided, it should be called 'defaults' - if > 1 mapping object is provided, it should be named after a model name (as provided in addModels()). For such cases, by having modelID as a vector of models modelID[1] is picked as the name for the mapping object.
R/plots.R
Outdated
| @@ -207,20 +207,31 @@ resetSearch <- function(pkgNamespaces) { | |||
| getMappingPlottingData <- function(study = study, modelID = modelID, featureID = featureID, testID = testID, libraries = NULL) { | |||
| mapping <- getMapping(study, libraries = libraries) | |||
|
|
|||
| # stop if mapping object contains only one element and is not called "defaults" | |||
There was a problem hiding this comment.
Conclusion from our Slack discussion: let's switch to consistently using modelID = "default", and not make it a hard requirement that a single mapping table be named "default" (this is recommended, but it's hard to predict future use cases)
There was a problem hiding this comment.
Correcting message from 1st commit below, first bullet point:
- study obj accepts multiple mappings. For one single element, it may be called whatever user wants it to be named (but in description of addMapping, it recommends being 'default'). Other mapping list elements must be named after modelIDs.
- accepts one single element, whatever it may be named (but in description of addMapping, it recommends being 'default') - It only accepts one mapping list element NOT named after modelIDs (e.g., 'default') - modelID[1] passed through plotStudy is the reference for multimapping calls. If a modelID[1] is available among mapping list element names, this mapping element will be used in the call. If not, but a 'default' mapping is available, this is used instead. If not and no 'default' is available, return(). - checkResults and checkEnrichment were modified from 'defaults' to 'default'. testCheck was updated to include checking on those error messages.
| if ("defaults" %in% names(results)) { | ||
| stop("The results cannot be shared using the modelID \"defaults\"") | ||
| if ("default" %in% names(results)) { | ||
| stop("The results cannot be shared using the modelID \"default\"") |
There was a problem hiding this comment.
Thanks for fixing this!
| expect_error_xl( | ||
| addResults(study, results = resultsDefault), | ||
| 'The results cannot be shared using the modelID \"default\"' | ||
| ) |
R/plots.R
Outdated
| mapping_name <- modelID[1] | ||
| } else { | ||
| return() | ||
| } |
There was a problem hiding this comment.
For the above code, is there a reason you can't do something like the following?
mapping <- getMapping(study, modelID = modelID, libraries = libraries)In other words, can we offload the logic of testing for valid modelIDs and the presence of a "default" option to getMapping()?
There was a problem hiding this comment.
If I got the point correctly, this statement below:
mapping_name <- names(mapping)[!names(mapping) %in% modelID]
gets whatever mapping name that is not in modelID. Mapping name "default" is recommended only in the documentation of addMapping, but user can call it whatever (s)he wants. Still, I am requesting the mapping list element to be named. Is that the issue, i.e., whether user can't simply provide a mapping with no name?
|
Also, don't forget to replace Line 402 in 5c069e9 OmicNavigator/inst/tinytest/testCheck.R Line 459 in 5c069e9 |
- requires mapping list elements to be named either as 'default' or a model name. This is checked in validate.R, and enforced by getElements() - Textual adaptations in addMapping()
|
Added NEWS bullet in ca0f35e to summarize the final behavior of this PR:
|
