From 1c3fb55d4aeb73e0296b49895c8bce58acc98500 Mon Sep 17 00:00:00 2001 From: Siddhesh Deodhar Date: Wed, 6 Mar 2024 21:00:31 -0500 Subject: [PATCH 1/7] Error if same arg is passed to aes and geom --- R/layer.r | 6 ++++++ tests/testthat/test-compiler-errors.R | 16 +++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/R/layer.r b/R/layer.r index 28298e364..0ddcc20f5 100644 --- a/R/layer.r +++ b/R/layer.r @@ -177,6 +177,12 @@ Layer <- gganimintproto("Layer", NULL, } else { aesthetics <- self$mapping } + + duplicate_aes <- intersect(names(aesthetics), names(self$aes_params)) + if(length(duplicate_aes) > 0) { + stop(paste("Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom:", + paste(duplicate_aes, collapse = ", "))) + } # Drop aesthetics that are set or calculated set <- names(aesthetics) %in% names(self$aes_params) diff --git a/tests/testthat/test-compiler-errors.R b/tests/testthat/test-compiler-errors.R index bd0c0a628..0ed3ef778 100644 --- a/tests/testthat/test-compiler-errors.R +++ b/tests/testthat/test-compiler-errors.R @@ -177,7 +177,6 @@ test_that("warning for _off param without clickSelects", { pointone = ggplot()+ geom_point(aes(x = wt, y = mpg, colour = disp), size = 10, - colour="red", alpha_off=0.6, colour_off="transparent", data = mtcars)) @@ -197,3 +196,18 @@ test_that("animint() is an error", { animint() }, "No arguments passed to animint. Arguments should include ggplots(1 or more) and options(0 or more)", fixed=TRUE) }) + +test_that("Same argument passed to aes and geom is an error", { + scatter <- ggplot()+geom_path(aes( + x=life.expectancy, + y=fertility.rate, + color=region, + group=country, alpha=population),alpha=0.5, + data=WorldBank) + viz <- list( + plot = scatter + ) + expect_error({ + animint2dir(viz, open.browser=FALSE) + }, "Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom: alpha") +}) \ No newline at end of file From ea897bc5f7f51647ab32550baf9285f2e9b2d144 Mon Sep 17 00:00:00 2001 From: Siddhesh Deodhar Date: Wed, 6 Mar 2024 23:04:05 -0500 Subject: [PATCH 2/7] Delete test --- tests/testthat/Rplot001.png | Bin 0 -> 8877 bytes tests/testthat/test-compiler-empty-data.r | 23 ---------------------- 2 files changed, 23 deletions(-) create mode 100644 tests/testthat/Rplot001.png diff --git a/tests/testthat/Rplot001.png b/tests/testthat/Rplot001.png new file mode 100644 index 0000000000000000000000000000000000000000..84b6189d6cbfd0a546ba9fe488b73f0e7dd44558 GIT binary patch literal 8877 zcmeI&F$+Oa6bJBg-=m% Date: Wed, 6 Mar 2024 23:08:25 -0500 Subject: [PATCH 3/7] Delete Rplot001.png --- tests/testthat/Rplot001.png | Bin 8877 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 tests/testthat/Rplot001.png diff --git a/tests/testthat/Rplot001.png b/tests/testthat/Rplot001.png deleted file mode 100644 index 84b6189d6cbfd0a546ba9fe488b73f0e7dd44558..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 8877 zcmeI&F$+Oa6bJBg-=m% Date: Wed, 13 Mar 2024 10:45:14 -0400 Subject: [PATCH 4/7] Updated place to perform duplicate params checks --- DESCRIPTION | 2 +- NEWS.md | 4 ++++ R/geom-.r | 18 ++++++++++++++++++ R/layer.r | 6 ------ tests/testthat/test-compiler-errors.R | 17 ++++++++++++++++- 5 files changed, 39 insertions(+), 8 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 6da0529b4..f544e69b0 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: animint2 Title: Animated Interactive Grammar of Graphics -Version: 2024.2.4 +Version: 2024.3.12 URL: https://animint.github.io/animint2/ BugReports: https://github.com/animint/animint2/issues Authors@R: c( diff --git a/NEWS.md b/NEWS.md index 4b56fddc4..c8da29895 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,7 @@ +# Changes in version 2024.3.12 (PR#119) + + - Add validation checks for duplicate args passed geom and aes + # Changes in version 2024.2.4 (PR#116) - Add validation checks for duplicate and missing args passed to animint. diff --git a/R/geom-.r b/R/geom-.r index f7d75dd4b..015d5efff 100644 --- a/R/geom-.r +++ b/R/geom-.r @@ -147,6 +147,17 @@ Geom <- gganimintproto("Geom", pre_process = function(g, g.data, ranges){ list(g = g, g.data = g.data) }, + + get_off_params = function(input_string) { + pattern <- "_off$" + # Return parameter name without the _off suffix + if (grepl(pattern, input_string)) { + result <- sub(pattern, "", input_string) + return(result) + } else { + return("") + } + }, ## Save a layer to disk, save and return meta-data. ## l- one layer of the ggplot object. @@ -174,6 +185,13 @@ Geom <- gganimintproto("Geom", ## e.g. colour. ## 'colour', 'size' etc. have been moved to aes_params g$params <- getLayerParams(l) + geom_params <- c(names(l$aes_params), sapply(names(l$extra_params), l$geom$get_off_params)) + duplicate_aes <- intersect(geom_params, names(g$aes)) + + if(length(duplicate_aes) > 0) { + stop(paste("Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom:", + paste(duplicate_aes, collapse = ", "))) + } ## Make a list of variables to use for subsetting. subset_order is the ## order in which these variables will be accessed in the recursive diff --git a/R/layer.r b/R/layer.r index 0ddcc20f5..28298e364 100644 --- a/R/layer.r +++ b/R/layer.r @@ -177,12 +177,6 @@ Layer <- gganimintproto("Layer", NULL, } else { aesthetics <- self$mapping } - - duplicate_aes <- intersect(names(aesthetics), names(self$aes_params)) - if(length(duplicate_aes) > 0) { - stop(paste("Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom:", - paste(duplicate_aes, collapse = ", "))) - } # Drop aesthetics that are set or calculated set <- names(aesthetics) %in% names(self$aes_params) diff --git a/tests/testthat/test-compiler-errors.R b/tests/testthat/test-compiler-errors.R index 0ed3ef778..07b07149c 100644 --- a/tests/testthat/test-compiler-errors.R +++ b/tests/testthat/test-compiler-errors.R @@ -175,7 +175,7 @@ test_that("no warning for position=stack without showSelected", { test_that("warning for _off param without clickSelects", { viz.point1 <- list( pointone = ggplot()+ - geom_point(aes(x = wt, y = mpg, colour = disp), + geom_point(aes(x = wt, y = mpg), size = 10, alpha_off=0.6, colour_off="transparent", @@ -210,4 +210,19 @@ test_that("Same argument passed to aes and geom is an error", { expect_error({ animint2dir(viz, open.browser=FALSE) }, "Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom: alpha") +}) + +test_that("alpha and alpha_off passed to aes and geom is an error", { + scatter <- ggplot()+geom_path(aes( + x=life.expectancy, + y=fertility.rate, + color=region, + group=country, alpha=population),alpha_off=0.5, + data=WorldBank) + viz <- list( + plot = scatter + ) + expect_error({ + animint2dir(viz, open.browser=FALSE) + }, "Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom: alpha") }) \ No newline at end of file From bce343040bec9ccc46f2436e606efca2293a9c14 Mon Sep 17 00:00:00 2001 From: Siddhesh Deodhar Date: Thu, 14 Mar 2024 20:08:25 -0400 Subject: [PATCH 5/7] Perform validation checks using grep --- R/geom-.r | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/R/geom-.r b/R/geom-.r index 015d5efff..06fe37eb1 100644 --- a/R/geom-.r +++ b/R/geom-.r @@ -148,17 +148,6 @@ Geom <- gganimintproto("Geom", list(g = g, g.data = g.data) }, - get_off_params = function(input_string) { - pattern <- "_off$" - # Return parameter name without the _off suffix - if (grepl(pattern, input_string)) { - result <- sub(pattern, "", input_string) - return(result) - } else { - return("") - } - }, - ## Save a layer to disk, save and return meta-data. ## l- one layer of the ggplot object. ## d- one layer of calculated data from ggplot_build(p). @@ -185,7 +174,8 @@ Geom <- gganimintproto("Geom", ## e.g. colour. ## 'colour', 'size' etc. have been moved to aes_params g$params <- getLayerParams(l) - geom_params <- c(names(l$aes_params), sapply(names(l$extra_params), l$geom$get_off_params)) + off_params <- sub("_off", "", grep("_off", names(l$extra_params), value = TRUE)) + geom_params <- c(names(l$aes_params), off_params) duplicate_aes <- intersect(geom_params, names(g$aes)) if(length(duplicate_aes) > 0) { From 887676702a76029ffa1bc5d80aabe02381c37181 Mon Sep 17 00:00:00 2001 From: Siddhesh Deodhar Date: Fri, 15 Mar 2024 22:03:18 -0400 Subject: [PATCH 6/7] Updates to error message and new test --- R/geom-.r | 13 +++++++++---- tests/testthat/test-compiler-errors.R | 21 ++++++++++++++++++--- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/R/geom-.r b/R/geom-.r index 06fe37eb1..ab70cf036 100644 --- a/R/geom-.r +++ b/R/geom-.r @@ -176,11 +176,16 @@ Geom <- gganimintproto("Geom", g$params <- getLayerParams(l) off_params <- sub("_off", "", grep("_off", names(l$extra_params), value = TRUE)) geom_params <- c(names(l$aes_params), off_params) - duplicate_aes <- intersect(geom_params, names(g$aes)) + duplicate_params <- intersect(geom_params, names(g$aes)) - if(length(duplicate_aes) > 0) { - stop(paste("Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom:", - paste(duplicate_aes, collapse = ", "))) + + if(length(duplicate_params) > 0) { + duplicate_on <- intersect(names(l$aes_params), duplicate_params) + duplicate_off <- setdiff(paste0(intersect(off_params, duplicate_params),"_off"), "_off") + duplicate_geom <- c(duplicate_on, duplicate_off) + stop(paste("Same argument cannot be passed to both aes and geom. Argument passed to aes:", + paste(duplicate_params, collapse = ", "), ". Arguments passed to geom:", paste(duplicate_geom, collapse = ", "), + ". The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom")) } ## Make a list of variables to use for subsetting. subset_order is the diff --git a/tests/testthat/test-compiler-errors.R b/tests/testthat/test-compiler-errors.R index 07b07149c..76f64b230 100644 --- a/tests/testthat/test-compiler-errors.R +++ b/tests/testthat/test-compiler-errors.R @@ -209,7 +209,7 @@ test_that("Same argument passed to aes and geom is an error", { ) expect_error({ animint2dir(viz, open.browser=FALSE) - }, "Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom: alpha") + }, "Same argument cannot be passed to both aes and geom. Argument passed to aes: alpha . Arguments passed to geom: alpha . The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom") }) test_that("alpha and alpha_off passed to aes and geom is an error", { @@ -217,12 +217,27 @@ test_that("alpha and alpha_off passed to aes and geom is an error", { x=life.expectancy, y=fertility.rate, color=region, - group=country, alpha=population),alpha_off=0.5, + group=country, alpha=population),clickSelects="country", alpha_off=0.5, data=WorldBank) viz <- list( plot = scatter ) expect_error({ animint2dir(viz, open.browser=FALSE) - }, "Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom: alpha") + }, "Same argument cannot be passed to both aes and geom. Argument passed to aes: alpha . Arguments passed to geom: alpha_off . The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom") +}) + +test_that("alpha_off without clickSelects is a warning", { + scatter <- ggplot()+geom_path(aes( + x=life.expectancy, + y=fertility.rate, + color=region, + group=country),alpha_off=0.5, + data=WorldBank) + viz <- list( + plot = scatter + ) + expect_warning({ + animint2dir(viz, open.browser=FALSE) + }, "geom1_path_plot has alpha_off which is not used because this geom has no clickSelects; please specify clickSelects or remove alpha_off") }) \ No newline at end of file From a242cc467aecc9ac2611cb03ec4522cf878b7eaa Mon Sep 17 00:00:00 2001 From: Siddhesh Deodhar Date: Sat, 16 Mar 2024 23:58:40 -0400 Subject: [PATCH 7/7] Updated error message and test --- R/geom-.r | 4 ++-- tests/testthat/test-compiler-errors.R | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/R/geom-.r b/R/geom-.r index ab70cf036..f7838fcd8 100644 --- a/R/geom-.r +++ b/R/geom-.r @@ -183,8 +183,8 @@ Geom <- gganimintproto("Geom", duplicate_on <- intersect(names(l$aes_params), duplicate_params) duplicate_off <- setdiff(paste0(intersect(off_params, duplicate_params),"_off"), "_off") duplicate_geom <- c(duplicate_on, duplicate_off) - stop(paste("Same argument cannot be passed to both aes and geom. Argument passed to aes:", - paste(duplicate_params, collapse = ", "), ". Arguments passed to geom:", paste(duplicate_geom, collapse = ", "), + stop(paste0("Same visual property cannot be defined in both aes and geom. Property defined in aes:", + paste0(duplicate_params, collapse = ", "), ". Property defined in geom:", paste0(duplicate_geom, collapse = ", "), ". The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom")) } diff --git a/tests/testthat/test-compiler-errors.R b/tests/testthat/test-compiler-errors.R index 76f64b230..f497b93a8 100644 --- a/tests/testthat/test-compiler-errors.R +++ b/tests/testthat/test-compiler-errors.R @@ -209,7 +209,7 @@ test_that("Same argument passed to aes and geom is an error", { ) expect_error({ animint2dir(viz, open.browser=FALSE) - }, "Same argument cannot be passed to both aes and geom. Argument passed to aes: alpha . Arguments passed to geom: alpha . The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom") + }, "Same visual property cannot be defined in both aes and geom. Property defined in aes:alpha. Property defined in geom:alpha. The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom") }) test_that("alpha and alpha_off passed to aes and geom is an error", { @@ -224,7 +224,7 @@ test_that("alpha and alpha_off passed to aes and geom is an error", { ) expect_error({ animint2dir(viz, open.browser=FALSE) - }, "Same argument cannot be passed to both aes and geom. Argument passed to aes: alpha . Arguments passed to geom: alpha_off . The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom") + }, "Same visual property cannot be defined in both aes and geom. Property defined in aes:alpha. Property defined in geom:alpha_off. The visual property needs only be defined in one place, so if it should be different for each rendered geom, but not depend on selection state, then it should be defined in aes; but if the property should depend on the selection state then it should be defined in geom") }) test_that("alpha_off without clickSelects is a warning", {