Conversation
726636d to
45b402b
Compare
R/display.R
Outdated
There was a problem hiding this comment.
I'm not quite sure about this function. Can you pull this (and related changes like get_display_fun out into a separate PR so we can discuss them apart from the more mundane changes to formatting etc.
There was a problem hiding this comment.
ah, basically this commit is the meat of it, the rest is format changes.
how can i “un-push” the other commit? or is it enough for you to be able to see that commit on its own?
|
Instead of Code that wants to provide multiple formats in a single output should just call display directly for now, I think. |
|
rather and i don’t get what you mean with your second line: |
|
Width and height metadata don't have any effect on svg or pdf, in any case - they're only used for png and jpeg. |
|
I don't think 'images' is a useful subcategory of outputs - as far as possible, anything we do should work for all output formats. |
60b8c7b to
5dd66da
Compare
|
hmm, currently those are convenience functions to faciliate sending multiple plots. if i get you right, you propose to
|
|
I'd be inclined to do without the convenience code for now, and write the multiple-plot-format code where it's going to be used. Then we can later work out what convenience methods, if any, we want to add to this. Besides the function names ( Unfortunately I don't have enough time to think through the API design carefully at the moment, because I'm getting ready to go to Pycon on Sunday. |
|
hmm, maybe this works for you. |
|
Can we put this on hold for the moment? I'll try to work out exactly what I want in a couple of weeks when I'm back from PyCon. In the meantime, I'd suggest making a PR for the functionality you want in IRkernel, without using any of this new convenience code, so we can see what that looks like. |
|
no convenience code anymore. well, except from not using mimetypes but format names. i’d really really like to see multiple formats in the kernel, and i think it would be much duplicated work to implement it in a different way. of course it can wait as i can use local copies to get what i want. |
|
What's wrong with using mimetypes? You can construct lists with mimetype keys: |
|
btw.: this is the change to the kernel code necessary to get multiple plot outputs based on this PR. (the other changes are updates to namespace and the notebooks with the new content) |
|
yeah, you’re right, but it’s getting too late for today. are you leaving tomorrow? if so, have a nice trip! PS: would that be the only thing needing change for you to accept both PRs? |
|
I'm leaving on Sunday, but there's still some stuff I need to prepare ;-). Thanks! I think I'd like to see an IRkernel PR that achieves the same without any change to IRdisplay for the time being. I don't think that's terribly difficult, the code should look something like this: data = list()
for (mimetype in names(image_formats)) {
format <- image_formats[[mimetype]]
tf <- tempfile(fileext = format$extension)
do.call(format$device, c(list(filename = tf), get_plot_options()))
replayPlot(plotobj)
dev.off()
if (format$binary){
data[mimetype] = base64encode(file)
} else {
data[mimetype] = readChar(file, ...)
}
}
display(data)That makes me think that the most important thing to add to IRdisplay is better knowledge about which mimetypes are binary, and perhaps a convenience wrapper to read a file as text/binary according to mimetype. But I haven't actually tried implementing that, so the implementation may turn up more things. |
|
I think one of the problems here is mixing
If we want to model this more or less like the the python kernel, then we need:
[Updated with informations on how plots are send (-> with a "display_data" message and not via a "execution_result")] |
|
i agree with most of this! but i’d rather like to go the following route:
why not use the ones from the i think
i could change the latter to
|
|
Re where to put the Code: I think the "get plotting output out to the frontend" code should not be in IRdisplay at all but RKernel. If you start using the repr package now, it should actually work (minus content send, as the default formatter are not implemented yet), as the interface is there. Having the kernel side, implementing display is IMO much cleaner, as it all becomes something like display <- function(obj, mimetype=NULL, raw=FALSE){
if (raw and mimetype == NULL) stop("raw output without a specified mimetype doesn't work..."
_send_display_data(obj, mimetype, raw)
}
display_png <- function(obj, raw=FALSE){
_send_display_data(obj, "image/png", raw)
}
_send_display_data <- function(obj, mimetype=NULL, raw=FALSE){
display_data = c() # I'm pretty sure there is a more R way of constructing a list in a loop :-)
if (raw != NULL){
if (mimetype == NULL) stop("raw output without a specified mimetype doesn't work..."
display_data = c(c(mimetype, obj))
}else if (mimetype is NULL){
# this could be a function in the kernel, as this is more or less what happens
# for all returned objects in the execution_result message
# get the list of all active formatters
# for formatter in active:
# display_data = c(display_data, c(formatter$mimetype, formatter$repr(obj)))
} else {
# get the formatter, no matter if active or not
display_data = c(c(formatter$mimetype, formatter$repr(obj)))
}
# base64 encode all binary data, aka images mimetypes
# -> should probably be a function in the kernel, as it is needed there as well for
# the execution_result message
# do whatever it takes to send the data in a `display_data` message
}[in python, there is also the possibility to send multiple objects at once, e.g. In the end, Re "where to put repr_png": having some default implementation directly in the the repr package sounds actually better than having the implementations in the kernel, as these are incentives for other package (knitr?) to use it and gain such things for free :-) |
|
so, i think this is it for now. the better/repr version can come after this, but now we have
|
|
so what do you say? pull this, then flesh out and switch to repr? |
|
Please hold off until I have time to dig in and review this, which probably won't be until after Pycon. Sorry for the delay. |
fd72330 to
1652567
Compare
|
so this is it: i basically maintained the API of the convenience functions here and added the mimetype-agnostic IRkernel/IRkernel#82 depends on |
R/display.R
Outdated
There was a problem hiding this comment.
Shouldn't this be an alternative branch for display (if it returns a display_data reply): if the data is a list, use this path else the current display?
There was a problem hiding this comment.
i’m going all python zen here: expicit is better than implicit, yet practicality beats purity.
this would hint at unifying them, but there’s one difference:
display_alternatives is more related to display1, because it knows about mimetypes. display is simply “do everything manually”, display1 and display_alternatives are “display things using mimetypes”, and display_* are convenience functions to save people from remembering all those mimetypes.
|
It looks like the only real difference between So with my decision-making hat on: let's drop We should, however, introduce a convenience function that takes an object and tries to call the various |
Hi, please voice your concerns about code organization/DRYness now, before i pull this in.