Delimited mr by gshotwell · Pull Request #154 · Crunch-io/rcrunch

gshotwell · 2017-11-01T18:03:57Z

Create Multiple Response variables from delimited text or categorical variables. The process this function uses to generate the MR is:

Pull data into R
Construct and upload subvariable definitions
Call makeMR to bind those subvariables into a MR variable
Hide original variable

codecov · 2017-11-01T18:04:00Z

Codecov Report

Merging #154 into master will decrease coverage by 0.12%.
The diff coverage is 65.51%.

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   89.21%   89.08%   -0.13%     
==========================================
  Files          92       92              
  Lines        5478     5507      +29     
==========================================
+ Hits         4887     4906      +19     
- Misses        591      601      +10

Impacted Files	Coverage Δ
R/make-array.R	`68.31% <65.51%> (-1.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdd0104...81f2797. Read the comment docs.

codecov · 2017-11-01T18:04:00Z

Codecov Report

Merging #154 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   90.04%   89.91%   -0.13%     
==========================================
  Files         105      104       -1     
  Lines        6466     6414      -52     
==========================================
- Hits         5822     5767      -55     
- Misses        644      647       +3

Impacted Files	Coverage Δ
R/make-array.R	`82.94% <100%> (+13.5%)`	⬆️
R/misc.R	`98.91% <100%> (-0.27%)`	⬇️
R/variable-update.R	`86.23% <0%> (-2%)`	⬇️
R/dataset.R	`93.28% <0%> (-1.57%)`	⬇️
R/variable.R	`82.6% <0%> (-0.73%)`	⬇️
R/AllGenerics.R	`92.59% <0%> (-0.52%)`	⬇️
R/cube-result.R	`99.38% <0%> (-0.02%)`	⬇️
R/progress.R	`100% <0%> (ø)`	⬆️
R/variable-as-methods.R
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7726bb...3144e12. Read the comment docs.

jonkeane

This is very exciting. I know that we've struggled with making this easy, and it will be great to be able to do this.

jonkeane · 2017-11-14T18:42:46Z

R/make-array.R

 }

+
+#' Create Multiple Response Variable from Delimited


Super nitpicky: Create a Multiple Response Variable from Delimited Lists or Create Multiple Response Variables from Delimited Lists

jonkeane · 2017-11-14T20:39:03Z

R/make-array.R

+#' @param unanswered Character string indicating non-response
+#' @param ... Other arguments to be passed on to [makeMR()]
+#'
+#' @return


Add a return string here

jonkeane · 2017-11-14T20:45:55Z

R/make-array.R

+#' @param delim The delimiter separating the responses
+#' @param name The name of the resulting MR variable
+#' @param selected A character string used to indicate a selection
+#' @param not_selected Character string identifying non-selection


Add references to the default values that are specified

I don't think "selected" and "not_selected" arguments are worth having. And I'm guessing that if you convert to the derived expression (using ~= on the server), you won't have room for defining subvariable category names anyway.

jonkeane · 2017-11-14T20:51:19Z

R/make-array.R

+        v <- as.vector(var)
+    } else {
+        halt(dQuote("var"), " must be a Categorical or Text Crunch Variable.")
+    }


Here, dQuote("var") could be dQuote(substitute(var)) to get the actual variable string that was given

jonkeane · 2017-11-14T21:02:17Z

R/make-array.R

+    ds <- loadDataset(datasetReference(var))
+    addVariables(ds, vardefs)
+    hide(var)
+    ds <- refresh(ds)


You can get rid of this refresh line, if you change line 122 to ds <- addVariables(ds, vardefs) and possible also change line 123 to var <- hide(var)

You can get rid of all of this if we accept #162 and you use that approach instead.

jonkeane · 2017-11-14T21:34:12Z

tests/testthat/test-make-array.R

+                c("not_selected", "selected", "No Data"))
+            expect_identical(as.vector(ds$mr_5$maple),
+                structure(c(2L, 2L, 1L, 1L), .Label = c("not_selected", "selected"
+                ), class = "factor"))


I think this would be more readable as factor(c("selected", "selected", "not_selected", "not_selected"), levels=c("not_selected", "selected"))

jonkeane · 2017-11-14T21:41:17Z

tests/testthat/test-make-array.R

+             unanswered = v[is.na(v)])
+         expect_equivalent(varDef, expected)
+    })
+    c("maple; birch", "oak; maple; birch", "birch; sugar maple", "maple butter; oak")


Is this line leftover?

jonkeane · 2017-11-14T21:43:06Z

tests/testthat/test-make-array.R

+             not_selected = "No",
+             unanswered = v[is.na(v)])
+         expect_equivalent(varDef, expected)
+    })


This expectation doesn't seem quite right: I would have expected "oak" to be matched a few times given those values.

jonkeane · 2017-11-14T21:47:02Z

tests/testthat/test-make-array.R

+    })
+
+    test_that("createSubvarDef generates the correct variable definition", {
+         v <- c("maple; birch", "oak; maple; birch", "birch; sugar maple", "maple butter; oak", NA)


it would be good to also test where one of the values is selected in all of the values.

jonkeane · 2017-11-14T21:49:08Z

R/make-array.R

+    vardefs <- lapply(cats, function(x) createSubvarDef(v, x, delim,
+        selected, not_selected, unanswered, missing = is.na(v)))
+    ds <- loadDataset(datasetReference(var))
+    addVariables(ds, vardefs)


I'm not sure this is avoidable, but maybe it would be good to add a check that these variables don't already exist / if they do rename them? It seems a bit problematic that someone could only ever use this command once on a variable (without going through and manually deleting the subvariables that were created).

How about just changing the subvariable names to varname_selection? In other words if the delimited variable was called trees then the subvariables would be trees_oak trees_maple etc. My preference would be to keep the function consistent so that it always named things in the same way and didn't change behaviour based of the other variables in a dataset.

varname_selection is nice, but it doesn't ensure uniqueness. A safer way is to create the variable in one request (not upload and then bind, which is not atomic). And in doing it in a single request, you may be able to omit aliases for the subvariables, in which case the backend should generate (guaranteed valid) aliases for them.

gshotwell · 2017-11-17T20:19:33Z

It looks like there's a zz9 bug which prevents creating a MR variable with derived subvariable definitions.

https://www.pivotaltracker.com/story/show/152975005 &
https://www.pivotaltracker.com/story/show/152975139

The ideal scenario is to create the multiple response variable definition in one step without having to first create the subvariables, but for the time being we probably need to use the two step process until the bug is resolved.

jonkeane · 2018-02-02T21:42:11Z

R/make-array.R

+    deriv <- zfunc("case", new_cat)
+    deriv$args[[2]] <- zfunc("is_missing", var)
+    deriv$args[[3]] <- zfunc("~=", var, buildDelimRegex(str, delim))
+    deriv$references <- list(name = str, alias = paste0(alias(var), "_", str))


We should probably check that these aliases are unique in the dataset. Currently the concatenate the source variable's alias with the item name (str).

It should be doable to check that this alias doesn't exist, and if it does do something else.

- Changed map names to accomodate no periods in Mongo - Added escape functionality for regex metacharacters

GShotwell added 6 commits October 31, 2017 16:24

Initial implementation of mrFromDelim

0326754

Initial implementation of mrFromDelim

80c987d

Merge remote-tracking branch 'origin/delimited_MR' into delimited_MR

cebacaf

Hide variables after upload

943ff7c

Merge remote-tracking branch 'origin/master' into delimited_MR

fd11238

Removed whitespace

81f2797

Expanded documentation

efed473

jonkeane suggested changes Nov 14, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into delimited_MR

208762b

nealrichardson mentioned this pull request Nov 15, 2017

Derive a replacement variable #162

Open

GShotwell added 3 commits November 16, 2017 10:01

Switched to Variable Definition

42ff9c9

Variable definition accomodates missing values

6a035ea

Updated tests

52479c2

jonkeane added 4 commits February 2, 2018 15:13

Proper ZCL for deriving MRs directly from other variables.

8aeb3c7

Updated docs

e57b227

Merge branch 'master' into delimited_MR

e047f6a

Man page optimization

36c5bf4

jonkeane reviewed Feb 2, 2018

View reviewed changes

GShotwell and others added 4 commits February 13, 2018 14:22

Merge remote-tracking branch 'origin/master' into delimited_MR

705a102

Regex handling

6b7fcb3

- Changed map names to accomodate no periods in Mongo - Added escape functionality for regex metacharacters

Merge from master

442c8e5

Rename function to makeMRFromText(); add NEWS

3144e12

nealrichardson merged commit f9cbe3c into master Mar 15, 2018

nealrichardson deleted the delimited_MR branch March 15, 2018 21:19

Conversation

gshotwell commented Nov 1, 2017

Uh oh!

codecov bot commented Nov 1, 2017

Codecov Report

Uh oh!

codecov bot commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gshotwell commented Nov 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 1, 2017 •

edited

Loading