Skip to content
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

A cognostic that is entirely NA causes a cryptic error message. #148

Open
jrounds opened this issue Mar 30, 2016 · 0 comments
Open

A cognostic that is entirely NA causes a cryptic error message. #148

jrounds opened this issue Mar 30, 2016 · 0 comments

Comments

@jrounds
Copy link

jrounds commented Mar 30, 2016

kv = lapply(1:10, function(i) list(k = i, v= i))
ddo0 = ddo(kv)
cogFn = function(i) list(cog1 = 1, cog2 = NA)
t = tempfile()
vdb_conn = vdbConn(t, autoYes=TRUE)
makeDisplay(ddo0, panelFn=function(v) plot(1,1), cogFn=cogFn, conn= vdb_conn, name="break things")
Error in data.frame(name = nms[i], tmp, stringsAsFactors = FALSE) : 
  arguments imply differing number of rows: 1, 0

If you are like me you are like what?

The bug bites here:
https://github.com/tesseradata/trelliscope/blob/1a651ad231546b592236cb7251fa31d1f7f1adc6/R/cognostics.R#L413

The issue is that when the column of cog is NA this line is empty:
tmp <- attr(x[[i]], "cogAttrs")

when it is empty the row count is mismatched on the next line:
data.frame(name = nms[i], tmp, stringsAsFactors = FALSE)

Two ideas present themselves:

  1. Catch and give a stop with a decent error message. Difficultly: easy.
    if(all(is.na(...))) stop("At least one cognostic of _name_ must not be NA")
  2. Make it work. Technically not broken? Maybe it is. Difficulty: little harder. Involves checking tmp and also making sure everything down streams of it still works with all NA. Maybe not worth it. An NA column has no decision-able information it is just taking up space.

I think it is worth fixing because you really need to back up and think "what did I do last?" to figure this one out. In my case I added a bunch of cognostics and one was always NA.
#1 sounds safest to me. Just catch it and give it a decent message. Want a pull request?

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

No branches or pull requests

1 participant