-
Notifications
You must be signed in to change notification settings - Fork 633
fixed problem with equal axes #223
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
Changes from 13 commits
f6f2105
86fbec5
31a1ef6
ba738b9
79257be
955da5e
2dc1453
7cefa02
5d9deca
b969dee
af4fd33
b7f1645
ad260ed
0a85c90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
context("Fixed coordinates") | ||
|
||
# Expect trace function | ||
expect_traces <- function(gg, n_traces, name) { | ||
stopifnot(is.ggplot(gg)) | ||
stopifnot(is.numeric(n_traces)) | ||
save_outputs(gg, paste0("coord_fixed-", name)) | ||
L <- gg2list(gg) | ||
all_traces <- L$data | ||
no_data <- sapply(all_traces, function(tr) { | ||
is.null(tr[["x"]]) && is.null(tr[["y"]]) | ||
}) | ||
has_data <- all_traces[!no_data] | ||
expect_equal(length(has_data), n_traces) | ||
list(traces = has_data, layout = L$layout) | ||
} | ||
|
||
# Data where x ranges from 0-10, y ranges from 0-30 | ||
set.seed(202) | ||
dat <- data.frame(xval = runif(40,0,10), yval = runif(40,0,30)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember that we decided not to use randomized data in our test suite, so that the image diff's remain meaningful. But I may have totally missed a discussion on the topic in the meantime... If so, please let me know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that is still correct. I recommend just copying and pasting the results of the random data into the test :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good. An example of this practice can be found in this test: https://github.com/ropensci/plotly/blob/master/tests/testthat/test-plotly-filename.R There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure Even better, use a built-in dataset such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Gotta go to graduation now! Will be back shortly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, @cpsievert and @13bzhang ! I have learned something today. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkcor : Does that mean we can keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, all good! |
||
|
||
# Force equal scaling | ||
p <- ggplot(dat, aes(xval, yval)) + geom_point() + coord_fixed() | ||
# Test | ||
test_that("coord_fixed() is translated to the right height-width ratio", { | ||
info <- expect_traces(p, 1, "force_equal_scaling") | ||
tr <- info$traces[[1]] | ||
la <- info$layout | ||
expect_identical(tr$type, "scatter") | ||
# height-width ratio check | ||
x_range <- range(p$data$xval, na.rm = TRUE) | ||
y_range <- range(p$data$yval, na.rm = TRUE) | ||
yx_ratio <- (y_range[2] - y_range[1]) / (x_range[2] - x_range[1]) | ||
expect_equal(la$height/la$width, yx_ratio * p$coordinates$ratio, tolerance = 0.10) | ||
}) | ||
|
||
# Equal scaling, with each 1 on the x axis the same length as y on x axis | ||
p <- ggplot(dat, aes(xval, yval)) + geom_point() + coord_fixed(1/3) | ||
# Test | ||
test_that("coord_fixed() is translated to the right height-width ratio", { | ||
info <- expect_traces(p, 1, "force_equal_scaling") | ||
tr <- info$traces[[1]] | ||
la <- info$layout | ||
expect_identical(tr$type, "scatter") | ||
# height-width ratio check | ||
x_range <- range(p$data$xval, na.rm = TRUE) | ||
y_range <- range(p$data$yval, na.rm = TRUE) | ||
yx_ratio <- (y_range[2] - y_range[1]) / (x_range[2] - x_range[1]) | ||
expect_equal(la$height/la$width, yx_ratio * p$coordinates$ratio, tolerance = 0.10) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coding the height/width is going to make for unpleasant htmlwidget experience. I think the best way to do it is pass this ratio to the HTMLwidget.resize method and impose the ratio on the height/width there. We should also provide a way to specify this ratio for non-ggplots