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

as.character(htmlwidget) gives error in the display system #18

Closed
jankatins opened this issue Mar 23, 2016 · 24 comments
Closed

as.character(htmlwidget) gives error in the display system #18

jankatins opened this issue Mar 23, 2016 · 24 comments

Comments

@jankatins
Copy link
Contributor

#devtools::install_github("renkun-ken/formattable")
df <- data.frame(
  id = 1:10,
  name = c("Bob", "Ashley", "James", "David", "Jenny", 
    "Hans", "Leo", "John", "Emily", "Lee"), 
  age = c(28, 27, 30, 28, 29, 29, 27, 27, 31, 30),
  grade = c("C", "A", "A", "C", "B", "B", "B", "A", "C", "C"),
  test1_score = c(8.9, 9.5, 9.6, 8.9, 9.1, 9.3, 9.3, 9.9, 8.5, 8.6),
  test2_score = c(9.1, 9.1, 9.2, 9.1, 8.9, 8.5, 9.2, 9.3, 9.1, 8.8),
  final_score = c(9, 9.3, 9.4, 9, 9, 8.9, 9.25, 9.6, 8.8, 8.7),
  registered = c(TRUE, FALSE, TRUE, FALSE, TRUE, TRUE, TRUE, FALSE, FALSE, FALSE),
  stringsAsFactors = FALSE)
library(formattable)

f <- formattable(df, list(
  age = color_tile("white", "orange"),
  grade = formatter("span",
    style = x ~ ifelse(x == "A", style(color = "green", font.weight = "bold"), NA)),
  test1_score = normalize_bar("pink", 0.2),
  test2_score = normalize_bar("pink", 0.2),
  final_score = formatter("span",
    style = x ~ style(color = ifelse(rank(-x) <= 3, "green", "gray")),
    x ~ sprintf("%.2f (rank: %02d)", x, rank(-x))),
  registered = formatter("span", 
    style = x ~ style(color = ifelse(x, "green", "red")),
    x ~ icontext(ifelse(x, "ok", "remove"), ifelse(x, "Yes", "No")))
))
w <- as.htmlwidget(f)
h <- htmlwidgets:::toHTML(w, standalone = FALSE, knitrOptions = NULL)
as.character(h)

Results in

Error: is.vector(vec) || is.factor(vec) is not TRUE
Traceback:

1. tryCatch({
 .     for (mime in getOption("jupyter.display_mimetypes")) {
 .         r <- mime2repr[[mime]](obj)
 .         if (!is.null(r)) 
 .             data[[mime]] <- r
 .     }
 . }, error = handle_error)

"
@jankatins
Copy link
Contributor Author

Not sure what is happening here and that's the biggest problem: the traceback isn't informative :-(

@jankatins
Copy link
Contributor Author

This is better:

obj = as.character(h)


namedlist <- function() setNames(list(), character(0))
data <- namedlist()
for (mime in getOption("jupyter.display_mimetypes")) {
    r <- repr:::mime2repr[[mime]](obj)
    if (!is.null(r)) 
        data[[mime]] <- r
}
Error: is.vector(vec) || is.factor(vec) is not TRUE
Traceback:

1. repr:::mime2repr[[mime]](obj)   # at line 7 of file <text>
2. repr_latex.character(obj)
3. repr_latex.logical(latex.escape.vec(obj), ...)
4. repr_vector_generic(latex.escape.names(obj), "\\item %s\n", "\\item[%s] %s\n", 
 .     "\\textbf{%s:} %s", enum.wrap = "\\begin{enumerate*}\n%s\\end{enumerate*}\n", 
 .     named.wrap = "\\begin{description*}\n%s\\end{description*}\n", 
 .     only.named.item = "\\textbf{%s:} %s")
5. latex.escape.names(obj)
6. latex.escape.vec(obj)
7. stopifnot(is.vector(vec) || is.factor(vec))
8. stop(sprintf(ngettext(length(r), "%s is not TRUE", "%s are not all TRUE"), 
 .     ch), call. = FALSE, domain = NA)

@flying-sheep
Copy link
Member

heh, i was faster 😉

so you could place a break point somewhere and check what class(obj) is

@jankatins
Copy link
Contributor Author

class(obj)
"html" "character"

Breakpoints in the kernel? How do I do that?

IMO there are multiple issues here:

  • each repr_xxx call should get wrapped in a try error so that other display methods are not affected by unrelated errors
  • the error should get a better traceback
  • logging, so that this error goes to the console and not to the user?

@flying-sheep
Copy link
Member

and while you’re at it, you could patch repr to do

if (!(is.vector(vec) || is.factor(vec)))
    stop('expected `vec` to be a vector or factor but it is a', paste(class(vec), collapse = ', '))

instead of stopifnot(is.vector(vec) || is.factor(vec))

@flying-sheep
Copy link
Member

Breakpoints in the kernel? How do I do that?

run IRkernel::main(conn_file) in RStudio

or place a browser() in it.

@flying-sheep
Copy link
Member

each repr_xxx call should get wrapped in a try error so that other display methods are not affected by unrelated errors

good idea to be defensive. not without logging a warning to the front end though! we definitely want people to report issues when this happens!

the error should get a better traceback

sure, if you have an idea how. R’s error handling is horrible, i’m so happy that it mostly works. (in RStudio it also only mostly works, so we’re in good company)

logging, so that this error goes to the console and not to the user?

as said: please let it go to both (but not break output altogether)

@jankatins
Copy link
Contributor Author

good idea to be defensive. not without logging a warning to the front end though! we definitely want people to report issues when this happens!

I think the python end does not. Both sides are a pain: in the above case, I really don't care about an error message, but I've also tried to debug non-showing output which happend due to a bug in the display function...

Maybe: print an short error to stderr (or whatever we have to do to let it show up as such in the notebook, probably a extra display call for a string) and display the traceback (if that's possible) in the console.

sure, if you have an idea how. R’s error handling is horrible, i’m so happy that it mostly works. (in RStudio it also only mostly works, so we’re in good company)

We get much better errors for the evaluated stuff, why does it work there?

@takluyver
Copy link
Member

I think the python end does not (show errors from display formatters)

We changed our approach to that in IPython, and IIRC it was in the direction of making the errors from formatters more visible. There was some frustration when previously hidden errors started showing up, but I think that's mostly the right way to go, otherwise people don't see what they expect, but there's no explanation as to why.

@flying-sheep
Copy link
Member

good. that’s always the way i prefer to go: fail early and fail often, don’t put band aids over deeper issues.

@jankatins
Copy link
Contributor Author

The only thin missing here is why the error in repr_latex is happening... -> why does it get something else than a vector/factor... @flying-sheep do you have an idea why this is happening and wht the proper fix should be?

@jankatins
Copy link
Contributor Author

BW, this is the new error:

ERROR while rich displaying an object: Error in latex.escape.vec(obj): expected `vec` to be a vector or factor but it is ahtml, character

Traceback:
1. repr_latex.character(obj)
2. repr_latex.logical(latex.escape.vec(obj), ...)
3. repr_vector_generic(latex.escape.names(obj), "\\item %s\n", "\\item[%s] %s\n", 
 .     "\\textbf{%s:} %s", enum.wrap = "\\begin{enumerate*}\n%s\\end{enumerate*}\n", 
 .     named.wrap = "\\begin{description*}\n%s\\end{description*}\n", 
 .     only.named.item = "\\textbf{%s:} %s")
4. latex.escape.names(obj)
5. latex.escape.vec(obj)
6. stop("expected `vec` to be a vector or factor but it is a", paste(class(vec), 
 .     collapse = ", "))

@flying-sheep
Copy link
Member

ahtml, character

there you go. if it was only character, it’d be a vector as well, but since the ahtml class got in there somehow, it isn’t.

and that’s good since we don’t want HTML in our LaTeX

@jankatins
Copy link
Contributor Author

I think the a comes from the error string, not the class. It's missing a space there :-( html is a class/structure in the html widget package.

IMO this is another reason for not including repr_xxxx.vector because that is simple too greedy: a "STRING" is also a vector and already needs special attention (here).

@flying-sheep
Copy link
Member

It's missing a space there

fixed

reason for not including repr_xxxx.vector

who said we’d do that? and would it even do sth.? i thought you need to do *.integer, …?

a "STRING" is also a vector and already needs special attention

there are no primitive/individual values in R, so we special case length=1 vectors in order not to create e.g. HTML lists for individual values.

@jankatins
Copy link
Contributor Author

who said we’d do that? and would it even do sth.? i thought you need to do *.integer, …?

What I mean is that https://github.com/IRkernel/repr/blob/master/R/repr_vector.r should be axed in favour of just return NULL (= repr_xxx.default()) and getting the output of print(x) in the text/plain message :-)

@flying-sheep
Copy link
Member

what? no. why?

@jankatins
Copy link
Contributor Author

[personal opinion ahead] Because I see no benefit for rich displaying primitives and it is nearer to the user experience of RStudio users. Oh, and last but not least: I find it prettier :-) [/]

@jankatins
Copy link
Contributor Author

Is there actually a way to prevent some function to exported/defined? Like I set options(jupyter.rich_primitives = FALSE) and don't get rich output for them?

@jankatins
Copy link
Contributor Author

Ok, there is:

ca <- structure(1, class = c("C", "A"))
baz <- function(x) UseMethod("baz", x)
baz.A <- function(x) "A"
baz.C <- function(x) {
    if (getOption("test", FALSE)){
        NextMethod()       
    } else {
        "C"
    }
}
options(test = FALSE)
baz(ca)
options(test = TRUE)
baz(ca)

@flying-sheep
Copy link
Member

[personal opinion ahead] Because I see no benefit for rich displaying primitives and it is nearer to the user experience of RStudio users. Oh, and last but not least: I find it prettier :-) [/]

OK, i find it prettier with rich display, and it’s non-trivial to include the text output into other displays. maybe for HTML/LaTeX escaping it and wrapping it in <pre></pre>/\begin{verbatim}\end{verbatim}?

…nah

Is there actually a way to prevent some function to exported/defined?

no, and i don’t know why primitives should be special-cased here.

if we do that, we also would have to make everything else toggleable. and we have to toggle at runtime, in every single function, since options can be changed at runtime


if you really want it, you can just override repr_*.logical/integer/…, right?

@jankatins
Copy link
Contributor Author

no, and i don’t know why primitives should be special-cased here.

No, IMO it's the other way around: we special case data.frames and plots because there a display as a table / image is useful.

That's at least the way for the python kernel, which uses obj.__repr__() (equivalent to print(obj) in R) per default and only when an object implements a ._repr_xxx_() supports a rich display.

@flying-sheep
Copy link
Member

i find free-form text representations of anything less useful. e.g. are spaces part of the values in the vector or separating them?

are those hard line breaks only visual or do they mean something? if just visual, isn’t it better to let a rich display engine handle reflow/soft linebreaking?

and as said before: in python, __repr__ often means “give me a copyable literal”, which isn’t the case in R.

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

3 participants