Skip to content

first pass at creating flatline variables #458

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

Merged
merged 10 commits into from
Jun 12, 2020
Merged

first pass at creating flatline variables #458

merged 10 commits into from
Jun 12, 2020

Conversation

gergness
Copy link
Contributor

@gergness gergness commented Jun 5, 2020

closes #456

Things I don't love, but felt like good compromises for a first pass. Documenting them here in case you don't agree.

  1. Only work on existing array variables. You can't group together arbitrary variables (I wanted to make code simpler) nor use a variable definition to create an ad-hoc variable (because of as.vector doesn't work on categorical array expression #457 and because I'd have to parse the expression, which seemed like too much work).

  2. Very limited handling of missingness. rowDistinct() can either ignore any missing value (the default) or treat all kinds of missings as a single category (na.rm = FALSE). You can't customize behavior when there are multiple types of missing. flatlineResponse() uses equality, so if all values are a single type of missing it will come back as a Selected/TRUE value, but if only some, or if there are multiple types of missing it will come back as No Data/NA.

  3. I probably need to finish as.* functions for CrunchExpressions #430, because your proposed exclusion filter of

(row_nunique(A) == 1 + row_nunique(B) == 1 + row_nunique(C) == 1) ≥ 2

can't be created from R currently, because it needs to have as.numeric() around the logical expressions (because the db doesn't like adding logical + numeric, whch after adding A+B we have a numeric), and you can't currently do as.numeric() on CrunchLogicalExpr.

@gergness gergness requested a review from malecki June 5, 2020 20:58
@lintr-bot
Copy link

tests/testthat/test-row-distinct.R:36:1: style: Lines should not be more than 100 characters.

ds$catarray$subvar1 == ds$catarray$subvar2 & ds$catarray$subvar3 == ds$catarray$subvar2,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@lintr-bot
Copy link

tests/testthat/test-row-distinct.R:36:1: style: Lines should not be more than 100 characters.

ds$catarray$subvar1 == ds$catarray$subvar2 & ds$catarray$subvar3 == ds$catarray$subvar2,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@lintr-bot
Copy link

tests/testthat/test-row-distinct.R:36:1: style: Lines should not be more than 100 characters.

ds$catarray$subvar1 == ds$catarray$subvar2 & ds$catarray$subvar3 == ds$catarray$subvar2,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #458 into master will increase coverage by 0.00%.
The diff coverage is 91.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #458   +/-   ##
=======================================
  Coverage   90.54%   90.55%           
=======================================
  Files         121      122    +1     
  Lines        6931     6943   +12     
=======================================
+ Hits         6276     6287   +11     
- Misses        655      656    +1     
Impacted Files Coverage Δ
R/row-distinct.R 91.66% <91.66%> (ø)

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 24d9187...25ab1ba. Read the comment docs.

@lintr-bot
Copy link

tests/testthat/test-row-distinct.R:56:1: style: Lines should not be more than 100 characters.

ds$catarray$subvar1 == ds$catarray$subvar2 & ds$catarray$subvar3 == ds$catarray$subvar2,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

tests/testthat/test-row-distinct.R:77:1: style: Lines should not be more than 100 characters.

​        expect_error(flatlineResponse(ds$catarray["subvar1"]), "Array must have more than 1 subvariable.")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@gergness gergness merged commit e9a1327 into master Jun 12, 2020
@gergness gergness deleted the flatline branch June 12, 2020 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rowwise n_unique
2 participants