-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multitest #6
Multitest #6
Conversation
- plotStudy checks modified to incorporate multiple tests - getPlottingData modified (multiple test results are provided as list) - addPlots @param modified to incorporate the "multiTest" entry - getPlottingData @return modified to indicate output for multiple tests man files updated accordingly. Tinytests were not
- new plots are provided for tinytest, one for plotType = "multiTest", called "plotMultiTest_sf", and one for plotType = c("multiFeature", "multiTest"), called "plotMultiTest_mf". - new tinytest sections created: "plotStudy (multitest)" and "getPlottingData (package, multitest)" - vignettes updated to indicate possibility of multiple testIDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. The first major thing to fix is removing gather()
to see if the jobs will pass then
Hmm,
I am having a funny error of “index out of bounds”, which seems to be happening on calling the using(ttdo) call – at least if I remove all else and keep only source("tinytestSettings.R") and using(ttdo) in the script, this already triggers the error call.
The only modifications I implemented are as follows:
Tests.R, functions plotMultiTest_sf and plotMultiTest_mf:
var_x <- data.frame(lapply(x$results, `[`, 2))
colnames(var_x)<- names(x$results)
var_x <- data.table(features = rownames(var_x), var_x)
var_x <- data.table::melt(var_x, measure.vars = c(2,3))
var_y <- data.frame(lapply(x$results, `[`, 3))
colnames(var_y) <- names(x$results)
var_y <- data.table(features = rownames(var_y), var_y)
var_y <- data.table::melt(var_y, measure.vars = c(2,3))
testPlot.R
Here, at least if I run manually, the following call gives me an error:
expect_identical_xl(
pkgDependencies,
"ggplot2, graphics, rlang, stats"
)
While the following call passes well:
expect_identical_xl(
pkgDependencies,
"data.table, ggplot2, graphics, rlang, stats"
)
But again, as indicated in the first line, if I run in console yy <- tinytest::run_test_file("inst/tinytest/testPlot.R"), this gives me the error of “index out of bounds”, and as such the commands described on pkgDependencies, from testPlot.R are not even evaluated.
Finally, if I test the plots as in Tests.R or the tinytests as in testPlot.R (including data.table among the package dependencies), all tests pass well.
If you have time prior to the meeting, we could call on that today (we can talk after the meeting as well). I did not upload to Git as I would prefer solving this issue prior to pushing.
Thanks!
Best,
M.
Marco R. Curado, PHD
Senior Data Scientist I
AbbVie Deutschland GmbH & Co. KG
R&D Information Research (IR)
Knollstraße
67061 Ludwigshafen
OFFICE +496215891779
CEL +491709618720
EMAIL ***@***.******@***.***>
abbvie.com<http://www.abbvie.com/>
This communication may contain information that is proprietary, confidential, or exempt from disclosure. If you are not the intended recipient, please note that any other dissemination, distribution, use or copying of this communication is strictly prohibited. Anyone who receives this message in error should notify the sender immediately by telephone or by return e-mail and delete it from his or her computer.
________________________________
From: John Blischak ***@***.***>
Sent: Donnerstag, 2. September 2021 19:33
To: abbvie-external/OmicNavigator ***@***.***>
Cc: Rocha Curado, Marco ***@***.***>; Mention ***@***.***>
Subject: [EXTERNAL] Re: [abbvie-external/OmicNavigator] Multitest (#6)
@jdblischak requested changes on this pull request.
Looking good. The first major thing to fix is removing gather() to see if the jobs will pass then
________________________________
In R/tests.R<#6 (comment)>:
@@ -259,6 +259,39 @@ testPlots <- function() {
xlab = "PC 1", ylab = "PC 2", main = "PCA")
}
assign("plotMultiFeature", plotMultiFeature, envir = parent.frame())
+ plotMultiTest_sf <- function(x) {
+ var_x <- data.frame(lapply(x$results, `[`, 2))
+ colnames(var_x)<- names(x$results)
+ features <- rownames(var_x)
+ var_x <- data.frame(features, gather(var_x))
+
+ var_y <- data.frame(lapply(x$results, `[`, 3))
+ colnames(var_y) <- names(x$results)
+ features <- rownames(var_y)
+ var_y <- data.frame(features, gather(var_y))
The tests are failing because of this call to gather(). I assume you are calling tidyr::gather(). tidyr isn't a dependency of OmicNavigator, so we can't use it in the test examples. Could you replace this with either stats::reshape() or data.table::melt()?
________________________________
In inst/tinytest/testPlot.R<#6 (comment)>:
@@ -30,7 +31,7 @@ pkgDependencies <- utils::packageDescription(
expect_identical_xl(
pkgDependencies,
- "ggplot2, graphics, rlang, stats"
+ "ggplot2, graphics, rlang, stats, tidyr"
As above, please don't add new package dependencies for the example test plots
|
…ta.table::melt in testplot.
The failing test is this one: OmicNavigator/inst/tinytest/testApp.R Lines 93 to 97 in 71d3cb7
The In general I think this simplistic test is no longer sufficient now that we have many more example custom plotting functions. Could you please delete that failing test and replace it with one test per plotting function? The first two are below: expect_identical_xl(
studies[[1]][["plots"]][[1]][["plots"]][[1]][["plotType"]],
"singleFeature"
)
expect_identical_xl(
studies[[1]][["plots"]][[1]][["plots"]][[2]][["plotType"]],
"multiFeature"
)
# add similar tests for your multiTest plots here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are passing! 🎉 I've made some minor suggestions for you to address. Thanks!
Thanks @curadomr! |
Implementation of multitest functionality (work-in-progress) : support for plotType = "multitest" to make test results data available for custom plotting functions