layout ggplots in a table #139
Conversation
Test to check for titles of 3 plots
| xlab("X Axis") + ylab("Y Axis") | ||
|
|
||
| plot_list <- list( | ||
| plot1 = plot1, |
There was a problem hiding this comment.
please avoid numbers in variable names (plot1 plot2 plot3)
please use informative names instead of numbers
the names should be human-readable, and help us understand what is the difference between them
also below you do titles[[1]] but titles[["name"]] would be preferable (again, names are easier to understand than numbers)
There was a problem hiding this comment.
for example plot names in the layout test could be
left, upper_right, lower_right
etc
| xlab("X Axis") + ylab("Y Axis") | ||
|
|
||
| data3 <- data.frame( | ||
| x = c(2), # x-coordinates of the dots |
ramping up on animint.js file by making temporary code changes and see how it changes the plots to understand how different functions in it work
add all plots in single row of a newly created outer table
used element variable which was not used earlier which led to failures locally. in this commit the plots are fixed in a single row of outer table, thus interactivity does not affect the relative positions of them.
knit-print fails for phantomjs
previously i put all plots in a single row of outer table, in this commit i put all plots in single column. i have also removed knit-print test because it had not passed on phantomjs and only passed for chromote in chromote testing branch
in the newly created outer table, add 2 plots per row and then make a new row. this implementation will be built upon to complete if it is the suitable.
if the user has given input of row and column in theme animint, plots will be put in that location. but user needs to mention rows and columns for all plots. if the user does not want that option older functionality will work but with a new change of 3 plots per row and then a new row.
test for putting plots in specific location of outer table by passing arguments of row and column in theme. i am working on making it compatible with theme as well
check if svg element exists in a particular location of outer table.
error message if user adds row and not column or vice versa
made a function to check if svg element exists at a specific location of the outer table. so this function will return true if plot exists at that location, false otherwise
put plots with fixed locations first if the user has passed both row and column arguments in theme for that plot, then put all other plots in available empty locations. if user passes only row or column argument then give error message
changed scope of implicitly global variables for global-varaibles test to pass. made the code more modular
Give error message in R session if only row or col is added in theme function.
changed from theme to theme_animint for taking row and col arguments, removed the code to show browser side error if the user fails to add only row or col. because that is not necessary now because code to show error message in R session itself is there now. also changed theme function to theme_animint in the test i had wrote for passing row and col arguments
check_svg function was for checking empty locations for rest of the plots when user puts rows and cols for some plots while not for others. this is no longer needed now because now the user has to select specific positions for all plots or for no plots at all
corrected the code to properly traverse the outer table to put the plots if the user choses not to use customized layout
|
hello @tdhock, one result of not putting hardcoded values for cell dimensions is that the empty cells collapse. for example, in the screenshot below it might appear that plot of 3 dots has been given argument (0,1) but the argument is actually (1,1). because plot of 2 dots is at (0,0) with rowspan=2 and cell at (0,1) is empty, it simply collapses and plot of 3 dots which is below it is effectively pushed upwards 
|
|
I think this is ok, because your example is not a typical use case (we don't care about how the renderer works in that case because we don't expect to ever need to use it like that). a better test case would be if you actually had a plot in 0,1 -- three plots total, one with rowspan=2, others with not. that is a typical use case. (we don't want to leave blank space on the page, but we do want to be able to specify plots in any arrangement we want, to fill up blank space) |
R/z_animintHelpers.R
Outdated
| for(rc in c("row", "col")){ | ||
| arc <- paste0("animint.", rc) | ||
| options_list[[rc]] <- if(arc %in% names(theme)){ | ||
| theme[[arc]] | ||
| } | ||
| } | ||
| options_list | ||
| } | ||
|
|
||
| getRowspanAndColumnspan <- function(theme){ | ||
| options_list <- list() | ||
| for(rscs in c("rowspan", "colspan")){ |
There was a problem hiding this comment.
the only difference between these two functions is the c("row","col") vs c("rowspan","colspan") so please refactor to emphasize that similarity. I would suggest just deleting both of the functions and instead using something like:
for( theme_attribute in c("row","col","rowspan","colspan" ){
animint.attribute <- paste0("animint.", theme_attribute)
plot.info[[theme_attribute]] <- plot$theme[[animint.attribute]]
}There was a problem hiding this comment.
okay. I would do that.
There was a problem hiding this comment.
merged the two functions into one
| @@ -0,0 +1,19 @@ | |||
| sendKey <- function(key, code, keyCode) { | |||
| remDr$Input$dispatchKeyEvent(type = "keyDown", key = key, code = code, windowsVirtualKeyCode = keyCode, nativeVirtualKeyCode = keyCode) | |||
There was a problem hiding this comment.
aren't these functions redundant with other functions? why do they need to be in a separate file? how are they different? please either delete, move, or add a comment here to explain why they need to stay here.
There was a problem hiding this comment.
aren't these functions redundant with other functions? why do they need to be in a separate file? how are they different? please either delete, move, or add a comment here to explain why they need to stay here.
I will take a look into that. I don't remember the exact reason I thought of to make a new file, maybe I missed to notice that a separate file was not necessary.
| test_that("bottom widget adds/remove points", { | ||
| expect_equal(get_circles(), list(10, 10)) | ||
| sendBackspace() | ||
| expect_equal(get_circles(), list(10, 5)) |
There was a problem hiding this comment.
this test file is important, why did you delete?
There was a problem hiding this comment.
| plot1=plot1, | ||
| plot2=plot2, | ||
| plot3=plot3, | ||
| plot4=plot4 |
There was a problem hiding this comment.
please avoid variable names with numbers like plot1 and instead use informative names
There was a problem hiding this comment.
yes. those files were accidentally committed.
|
|
||
| number_of_points <- length(points) | ||
| tables <- getNodeSet(info$html, "//table[@style='display: inline-block;']") | ||
| svgs <- getNodeSet(info$html, "//svg[contains(@id,'')]") |
There was a problem hiding this comment.
what are threeplots.R and fourplots.R supposed to test? can you please rename these files to emphasize what part of the code they are supposed to test?
please add a test that rowspan=2 works, as that is the main new useful feature of this PR. (colspan=2 can kind of be achieved in the current code, if the window is sized correctly)
There was a problem hiding this comment.
what are threeplots.R and fourplots.R supposed to test? can you please rename these files to emphasize what part of the code they are supposed to test? please add a test that rowspan=2 works, as that is the main new useful feature of this PR. (colspan=2 can kind of be achieved in the current code, if the window is sized correctly)
yes. I remember you had told me that at the start of this PR to give informative names for plots. I think I accidentally selected that test files while making a commit to correct any indentation changes I might have made. yes, I will add tests to test for multiple possibilities including rowspan
updated the code for handling rowspan without creating extra columns and pushing right cells further right as it did earlier.
|
Hello @tdhock , the code before my latest commit had one limitation for rowspan because of the way in which even the current code generates the html table. it creates a new row and then adds "n" number of columns to that row. it repeats the process for each row. the nested loops for doing that starts from this line : animint2/inst/htmljs/animint.js Line 2098 in 9624583 The result of such logic when rowspan is added can be shown below by a generic html table below - 
As seen in the table above, because of one rowspan, row 2 generates one column extra and outside the table. but the last commit attempts to mitigate this using a dynamic programming approach to save how many rowspans we have seen in a specific row towards the left and then simply pushes the plots towards the left by that much to bring them to the actual location intended by the user . it still creates extra columns like we see in the screenshot above but they collapse because they are empty. I have ran few tests to check and will write more and then commit them. |
|
wow dynamic programming |
I meant that the way I am creating the outer table adds extra column in a row for each cell in that row occupied by rowspan from above cells. for example let us consider the screenshot of the generic html table(0 indexed ) below which replicates my implementation of the outer table: 
now if the user wants to add plot at (1,1) it will actually get added in the second last square cell of the row 1, because the entire row is shifted due to rowspan. the location user actually wants from (1,1) is labelled as (1,0) in our case. so we need to reduce all column values in row 1 by 1 due to that 1 rowspan. if there are 2 rowspans we need to decrease column values by 2. but we need to consider only rowspans towards our left. that is where we can use a grid which keeps track of how many rowspans are there on left of each cell in that row. I will add tests soon that check for multiple possibilities including rowspan for the updated code which rectifies the issue above. |
|
this is getting too complicated. I think we should simplify the interface to ease maintenance of the code. |
|
what about only allowing user to define rowspan and colspan for each plot (not row nor col), and then we define "tr" in animint? animint(
"tr",
ggplot()+...
"tr",
ggplot()+...
)I think that would be even easier for maintenance and for users can you implement that please? |
removed unnecessary lines and added test to plot 9 plots, some with rowspans
refactored the code to make clear distinction between the code of customized plot layout and non customized plot layout. added one test
I have refactored the code to make the code of customized plot layout and code of non customized plot layout more distinct. even if we remove the option of row and column and just keep rowspan and colspan, we will still face issue of shifted plots due to rowspan like this - 
when we actually want the plots like this- 
|
|
I am using a grid which stores the count of cells towards the left occupied by rowspans from cells above them. every plot shifts to the left by that much value to compensate for rightwards shift due to these rowspans because of the way I am creating the outer table using 2 nested loops. the grid for 6 plots screenshot I have pasted in post above is like this- 
I will add comments in the code to make the use of each of the new function more clear |
|
Maybe I don't understand the issue you described as "even if we remove the option of row and column and just keep rowspan and colspan, we will still face issue of shifted plots due to rowspan like this - when we actually want the plots like this-" ? If I understand correctly, your issue is that when you have the first td with rowspan=2, you don't see a way of specifying the next column? My proposal is to not let the user specify row/col and instead the user will specify, for example animint(
"tr",
ggplot()+theme_animint(rowspan=2)+ggtitle("first td in first tr, rowspan=2"),
ggplot()+ggtitle("second td in first tr"),
"tr",
ggplot()+ggtitle("first td in second tr")
)would generate something like the following HTML / table, which I think would allow the user to fill in the next column, thus solving your issue, right? I believe this API would be more user-friendly and also easier to maintain. If that does not solve your issue, please explain in more detail. |
|
yes. I got that you are proposing to remove the option of giving row and column in theme_animint. but I have a query regarding its implementation. please take a look at the following function which creates outer table if user does not want the option of dedicated location- animint2/inst/htmljs/animint.js Line 2153 in 700f1d9 in the approach you are suggesting, will we create a outer table in the same way that the current code does ? using D3 javascript, first create row in the outer loop and then start adding columns to that row in the inner loop ? with added attributes of rowspan and colspan ? if yes, then we would still need to keep the count of rowspans in each row. and then reduce that much columns from available columns for each row while putting the plots in the loop below: animint2/inst/htmljs/animint.js Line 2168 in 700f1d9 the pushed columns outside the rectangle due to rowspans would simply collapse. |
|
hi @siddhesh195 I think your construct_outer_table function is un-necessarily complex. please see an alternative in #153 which is much simpler. If you agree, please close this PR and push updates to the other one. |
This pull request is a task that is a part of GSoC 2024. As mentioned in the issue #115, in the current implementation of animint2, ggplots appear one after another in the rendered HTML code and visual layout is determined interactively, by resizing/zooming the window. This PR aims to add a functionality for defining the layout so that we can arrange the ggplots in a grid in a predefined manner.
Closes #115