please give me feedback on these issues

[Faheem Mitha]

Hi,

1) I posted a patch on

[aesthetic inheritance and annotation_custom]
(https://github.com/hadley/ggplot2/issues/756)

see the last message. I reproduce this patch below. As mentioned
there, this patch fixes the issue, and appears to introduce no
regressions that I can see. At least I ran the unit tests and the
visual tests and they gave no errors.

If there are problems with the patch, i.e. code that it breaks, or if
there is some conceptual problem with it, I'd like to know.

NOTE: This code does not work with Winston Chang's example in the issue,
namely

###############################################################################

library(ggplot2)
library(grid)
len = 2
d <- data.frame(r = c( 6.279072, 2.995998, 8.193851, 11.274669),
f1 = c(rep("L", len), rep("H", len)), f2 = rep(c("A", "B"), len))
p3 <- ggplot(data.frame(foo = numeric(0)))
p3 <- p3 + geom_point(data = d, aes(x = f1, y = r, color = f2, group = f2))
p3 = p3 + annotation_custom(circleGrob())library(ggplot2)
library(grid)
len = 2
d <- data.frame(r = c( 6.279072, 2.995998, 8.193851, 11.274669),
f1 = c(rep("L", len), rep("H", len)), f2 = rep(c("A", "B"), len))
p3 <- ggplot(data.frame(foo = numeric(0)))
p3 <- p3 + geom_point(data = d, aes(x = f1, y = r, color = f2, group = f2))
p3 = p3 + annotation_custom(circleGrob())
#################################################################################

It seems there is nothing like this in the test code.

however, this is because the code does not replace the first data set
unless it is a waiver object. Perhaps the solution is also to discard
data frames that contain no data? I'm not sure. If the general
approach is considered acceptable, then the patch would be easy to
modify

Note that it does render, but without the circle.

2) See

[ggplot_gtable creates blank display]
(https://github.com/hadley/ggplot2/issues/809)

and the Stack Overflow question

[Why does this R ggplot2 code bring up a blank display device?]
(http://stackoverflow.com/q/17012518/350713)

This looks like something that would be fixable, but I don't
understand enough about what is going on to attempt to do so. Also, it
looks like it probably requires a bunch of fixes in different
locations.

3) See

[multiple calls to annotation_custom fail in certain cases]
(https://github.com/hadley/ggplot2/issues/817)

and the Stack Overflow question

[calling `str` on a object errors out if the ggplot2 package is loaded]
(http://stackoverflow.com/q/17127339/350713)

As mentioned therein, the error I was pursuing in the latter part of
this issue looks unlikely to be causing the issue, but is still
something that in my opinion should be fixed.

Regards, Faheem Mitha

## patch for https://github.com/hadley/ggplot2/issues/756
--- a/R/panel.r
+++ b/R/panel.r
@@ -43,10 +43,18 @@ train_layout <- function(panel, facet, d
# caused problems when they had names of aesthetics (like colour or group).
#
# @param panel a trained panel object
-# @param the facetting specification
+# @param facet the facetting specification
# @param data list of data frames (one for each layer)
# @param plot_data default plot data frame
map_layout <- function(panel, facet, data, plot_data) {
+ ## if plot_data is itself a waiver object, then select the first
+ ## non-waiver element of data as the new default
+ if(is.waive(plot_data))
+ {
+ firstnonwaivepos = match(TRUE, lapply(data, function(x) !is.waive(x)))
+ if(!is.na(firstnonwaivepos))
+ plot_data = data[[firstnonwaivepos]]
+ }
lapply(data, function(data) {
if (is.waive(data)) data <- plot_data
facet_map_layout(facet, data, panel$layout)

[Hadley Wickham]

Hi Faheem,

Unfortunately Winston and I are currently busy on other projects, so
we're unlikely to be able to get back to you in the near future. But
we will try and plan some time to work on ggplot2 by the end of the
year.

Hadley

> --
> You received this message because you are subscribed to the Google Groups
> "ggplot2-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ggplot2-dev...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>



--
Chief Scientist, RStudio
http://had.co.nz/

[Faheem Mitha]

On Thu, 18 Jul 2013, Hadley Wickham wrote:

> Hi Faheem,
>
> Unfortunately Winston and I are currently busy on other projects, so
> we're unlikely to be able to get back to you in the near future. But
> we will try and plan some time to work on ggplot2 by the end of the
> year.
>
> Hadley

Hi Hadley,

Thank you for your message. I realise you are busy, but I would appreciate
at least a comment (I'm not asking for a formal patch review) as to
whether the patch for #756 is on the right track, in your opinion.

Regards, Faheem

[Hadley Wickham]

> Thank you for your message. I realise you are busy, but I would appreciate
> at least a comment (I'm not asking for a formal patch review) as to whether
> the patch for #756 is on the right track, in your opinion.

It looks ok from a quick glance, but you're more likely to get useful
feedback if you can provide a few minimal test cases, and not write
quite so much. A separate pull request would be a good way to
proceed.

Hadley

[Faheem Mitha]

On Thu, 18 Jul 2013, Hadley Wickham wrote:

>> Thank you for your message. I realise you are busy, but I would appreciate
>> at least a comment (I'm not asking for a formal patch review) as to whether
>> the patch for #756 is on the right track, in your opinion.
>
> It looks ok from a quick glance, but you're more likely to get useful
> feedback if you can provide a few minimal test cases, and not write
> quite so much. A separate pull request would be a good way to
> proceed.

The comments in that issue were partly in case anyone was wondering about
the thinking behind that patch, partly to remind myself what the setup was
on returning later to the issue. The sorrounding logic is relatively
complex, and almost undocumented outside the source code; some design
notes would probably be useful for people who want to work on ggplot2.

Regardless, these comments can be ignored if desired.

My understanding of a pull request is that it represents a finished patch.
This patch is not finished. Specifically, it breaks the example that
Winston gave, mentioned in the first message on this thread, and
reproduced below.

Does this test need to pass? If so, this patch needs to be adjusted. There
may also be other edge cases that I have missed.

I guess my immediate question is whether a pull request represents a
finished patch, or if it can also be used for an unfinished patch that
needs further refinement and testing. If the latter, then I can submit a
pull request, with the understanding that it does not represent a finished
patch.

Regards, Faheem

[Hadley Wickham]

> I guess my immediate question is whether a pull request represents a
> finished patch, or if it can also be used for an unfinished patch that needs
> further refinement and testing. If the latter, then I can submit a pull
> request, with the understanding that it does not represent a finished patch.

A pull request is the best place for discussing changes to code,
whether complete or not.

[Faheem Mitha]

On Fri, 19 Jul 2013, Hadley Wickham wrote:

>> I guess my immediate question is whether a pull request represents a
>> finished patch, or if it can also be used for an unfinished patch that needs
>> further refinement and testing. If the latter, then I can submit a pull
>> request, with the understanding that it does not represent a finished patch.
>
> A pull request is the best place for discussing changes to code,
> whether complete or not.

Ok, I'll submit a pull request, but can you comment whether the patch
needs to work with Winston's example, please? Thanks.