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

Transition to the makeContent hook (in order to make the codebase eligible to exercise grid.force) #44

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Anirban166
Copy link
Collaborator

@Anirban166 Anirban166 commented Jul 7, 2021

Includes refactoring of:

  • dlgrob (grob to gTree) and drawDetails (drawDetails.dlgrob to makeContent.dlgrob)
  • draw.polygons and draw.rects
  • far.from.other.borders, project.onto.segments, calc.boxes and empty.grid

…eContent hook (grob is changed to gTree now)
…shape grobs as an extra (non-metadata) attribute (using a list object) of the data frame passed around
@Anirban166 Anirban166 marked this pull request as draft July 7, 2021 03:02
@Anirban166 Anirban166 force-pushed the makecontent-transition branch from ae45330 to feac076 Compare July 7, 2021 03:06
@Anirban166 Anirban166 force-pushed the makecontent-transition branch from e163675 to 807d0dd Compare July 7, 2021 03:22
@Anirban166
Copy link
Collaborator Author

Current approach at 807d0dd doesn't work for the shapeGrobs (only label is drawn) possibly because my attribute is a list object, and a gList strictly accepts only grobs.

The most obvious workaround (as I thought) would be to unwrap the list i.e. extract the individual grobs and then as comma separated items, put them inside as arguments within the gList. Here's a reproducible example to test the same, demonstrating what the makeContent method would look like, except that I am creating and assigning all the grobs in the function itself: (with context to this transition, the shapeGrobs are created within another function, i.e. either draw.polygons or draw.rects, and hence they need to be transferred via an attribute of the data frame passed around, acting as an argument to both)

library(grid)
textbox <- function(label, name = NULL, gp = NULL, vp = NULL) {
  gt <- gTree(label = label, name = name, gp = gp, vp = vp, cl = "textbox")
  grid.draw(gt)
}
makeContent.textbox <- function(x) {
  groblist <- list()
  tg <- textGrob(x$label, name = "text")
  groblist[[1]] <- roundrectGrob(width = 1.5*grobWidth(tg), height = 1.5*grobHeight(tg), name = "box")
  groblist[[2]] <- roundrectGrob(width = 2.5*grobWidth(tg), height = 2.5*grobHeight(tg), name = "box")
  setChildren(x, eval(bquote(gList(..(groblist), tg), splice = TRUE)))
}
grid.newpage()
t <- textbox("test", name = "tb")

image

@Anirban166
Copy link
Collaborator Author

This, however, doesn't work as expected for my main case (currently investigating as to what could be the reason) - I keep getting this: (even though I used unlist to convert the list to a vector)
image

makeContent.dlgrob <- function(x, recording) {
  cm.data <- x$data
  cm.data$x <- convertX(unit(cm.data$x,"native"),"cm",valueOnly=TRUE)
  cm.data$y <- convertY(unit(cm.data$y,"native"),"cm",valueOnly=TRUE)
  cm.data$groups <- factor(cm.data$groups)
  levs <- unique(cm.data[,c("groups","colour")])
  code <- as.character(levs$colour)
  names(code) <- as.character(levs$groups)
  cm.data <- ignore.na(cm.data)
  if(is.null(cm.data$label)){
    cm.data$label <- cm.data$groups
  }
  cm.data <- apply.method(
    x$method,
    cm.data,
    debug=x$debug,
    axes2native=x$axes2native)
  if(nrow(cm.data)==0)return()
  colour <- cm.data[["colour"]]
  cm.data$col <- if(is.null(colour)){
    code[as.character(cm.data$groups)]
  } else {
    colour
  }
  defaults <- list(hjust=0.5,vjust=0.5,rot=0)
  for(p in names(defaults)){
    if(!p %in% names(cm.data))cm.data[,p] <- NA
    cm.data[is.na(cm.data[,p]),p] <- defaults[[p]]
  }
  cm.data <- unique(cm.data)
  gpargs <- c("cex","alpha","fontface","fontfamily","col")
  gp <- do.call(gpar,cm.data[names(cm.data)%in%gpargs])
  if(x$debug){
    print(cm.data)
  }
  text.name <- paste0(
    "directlabels.text.", x$name)
  tg <- with(cm.data, textGrob(
    label,x,y,hjust=hjust,vjust=vjust,rot=rot,default.units="cm",
    gp=gp,
    name=text.name))
  sg <- unlist(attr(x$data, 'shapeGrobs'))
  #browser()
  setChildren(x, eval(bquote(gList(..(sg), tg), splice = TRUE)))
}

@Anirban166
Copy link
Collaborator Author

The problem strictly lies with the shape grobs (polygons/rectangles), as for example, using a method which does not incorporate these boundary boxes around the labels or one that does not call draw.polygons/draw.rects works fine and can make full use of grid.force(): (i.e. to say, textGrob is being rendered fine)

image

# Example taken from https://github.com/Anirban166/GSoC21-Tests#medium-test
library(ggplot2)
library(directlabels)
alpha.data <- data.frame(size = c(1, 2, 3, 4, 5), power = c(1, 1.25, 1.75, 2.5, 3.5), type = "Alpha")
beta.data <- data.frame(size = c(1, 2, 3, 4, 5), power = c(1.5, 3, 5, 7.25, 9.5), type = "Beta")
gamma.data <- data.frame(size = c(1, 2, 3, 4, 5), power = c(2, 4, 6.5, 10, 13.5), type = "Gamma")
df <- rbind(alpha.data, beta.data, gamma.data)
g <- ggplot(df, aes(size, power, color = type)) + geom_line() + xlim(0, 6) + labs(x = "Particle Size", y = "Penetration Power") + coord_cartesian(clip = "off")
direct.label(g, method = "last.qp")
library(grid)
grid.force()
grid.ls(fullNames=TRUE)

On the other hand, methods which use polygons/rectangles run without an error for the initial approach: (807d0dd)

image

# Example taken from https://github.com/tdhock/directlabels/pull/41
df <- data.frame(i=1:10, label="more
than
one
line
really
a
lot")
library(ggplot2)
gg <- ggplot(df, aes(i, i, color=label))+
  geom_line()
directlabels::direct.label(gg, "right.polygons")
grid::grid.force()
grid::grid.ls(fullNames = TRUE)

Combined with the fact that they are showing the remaining forced grobs, (no shapeGrobs and no error means those grobs must not exist) this made me think - are the shapeGrobs even available inside makeContent, or are they collected properly within draw.polygons/rects?

As it turns out, the grob list is in fact empty: (ran with the previous approach / changes in the last comment to trigger the error)
image

@Anirban166
Copy link
Collaborator Author

Putting a browser() call inside the loop helped, as it turns out the value for the items (only one here, nrow == 1) in the list persists, but then gets discarded altogether as the assignment of that list to the attribute lies outside the scope of the loop.

d3a
d3b

@Anirban166
Copy link
Collaborator Author

Putting a browser() call inside the loop helped, as it turns out the value for the items (only one here, nrow == 1) in the list persists, but then gets discarded altogether as the assignment of that list to the attribute lies outside the scope of the loop.

The assignment would work as normal (accessible outside the scope of the loop) within a regular for-loop but I realized a while ago that it was looping through the number of rows in the data frame with an environment constructed from it, i.e. using with. This creates the difference, for instance::

d <- data.frame(a = 1:3, b = 4:6, c = 7:9)
l <- list()
for(i in 1:nrow(d)) { l[[i]] <- i } # case 1
l # list with expected items: [[1]] 1 [[2]] 2 [[3]] 3
for(i in 1:nrow(d)) with(d[i, ], { l[[i]] <- i }) # case 2
l # list with 0 items

As a solution for the second case, I'm using the <<- operator, as scoping assignment would do the job (like it did here - in fond memory of last year)

However, makeContent still doesn't have access to the shape-grobs as because the attribute isn't being attached to the data frame (i.e. NULL values were being assigned in the gTree for sg all this while)

@Anirban166
Copy link
Collaborator Author

Anirban166 commented Jul 11, 2021

While the approach for returning the data frame is okay in general, the way the functions are being called by each other, the data frame returned by draw.polygons/rects probably isn't being used. For instance, consider:

d <- data.frame(a = 1:3, b = 4:6, c = 7:9)
f <- function(d) { 
  attr(d, 'newattrib') <- 1 
  return(d)
}

In order to obtain the attribute, the data frame returned by use of this function must be used:

attr(f(d), 'newattrib') # 1 

Otherwise, it would return NULL, much like what I'm experiencing for my case. Thought to use the super assignment operator, (like f <- function(d) { attr(d, 'newattrib') <<- 1; d }; f(d); attr(d, 'newattrib') # 1 for the case above) but it won't be of use here since the functions are in separate file-function scopes.

@Anirban166
Copy link
Collaborator Author

In the current setup, draw.polygons (or draw.rects) would be at the end of the function call chain, since they are the ones that generate grobs via grid.* calls, and that's what was required to be at the last stage previously. But for the transition, the draw.* functions must return their data frames to makeContent.dlgrob (can't call them within makeContent directly either) to retrieve the grobs, and then to subsequently assign them as children to the gTree therein.

@tdhock @bbolker what approach would you guys suggest to achieve this?

@tdhock
Copy link
Owner

tdhock commented Jul 15, 2021

first of all this is a great start, thanks for all the detailed debugging info.
second of all, you may want to change that code which does for(every row)grid.polygon(one polygon based on that row)
to a single polygonGrob call (which draws all of the polygons) without the for loop (should be possible).
finally I'm not sure about that issue about the "end of the function call chain." maybe you can do some re-arranging of the grobs at the end of the function call chain?

…from a list (values collected inside the for loop), and with separate ids for each row to distinguish between separate polygons and avoid contiguous drawing
Anirban166 added a commit that referenced this pull request Jul 22, 2021
…rts', include the grid version specification as an update, so as to have no conflicts with #44 (makecontent-transition branch)
@Anirban166
Copy link
Collaborator Author

first of all this is a great start, thanks for all the detailed debugging info.

Thanks, and you're welcome!

second of all, you may want to change that code which does for(every row)grid.polygon(one polygon based on that row)
to a single polygonGrob call (which draws all of the polygons) without the for loop (should be possible).

Done (also, there was a conflict with the description, so I updated that in master to match with the file here)

Key was to use id/id.lengths in order to distinguish between and separate the polygons (documentation reference for id: "A numeric vector used to separate locations in x and y into multiple polygons. All locations with the same id belong to the same polygon.") we are creating for each row. Otherwise, all the points (given by numeric vectors for x and y) would form only one single polygon, which is drawn consecutively from one (x,y) position to another (since there are no other options to specify breaks for those parameters, specifying an id seems like the way to go)

…s (or the total number of x/y values for one polygon based on a row)
… so as to use them in the call to polygonGrob() outside it
@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@b543ce1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #44   +/-   ##
=========================================
  Coverage          ?   44.45%           
=========================================
  Files             ?        8           
  Lines             ?     1046           
  Branches          ?        0           
=========================================
  Hits              ?      465           
  Misses            ?      581           
  Partials          ?        0           

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 b543ce1...702dc83. Read the comment docs.

…ints in a row, and obtain them outside in polygonGrob (done with pg here)
@Anirban166 Anirban166 force-pushed the master branch 2 times, most recently from 51adde8 to 10e243b Compare June 17, 2022 05:03
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.

3 participants