Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

fixed problem with equal axes #223

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f6f2105
fixed problem with equal axes
13bzhang May 15, 2015
86fbec5
add testthat test for my new features
13bzhang May 16, 2015
31a1ef6
fixed the expect_traces function
13bzhang May 19, 2015
ba738b9
add space to the end of test
13bzhang May 19, 2015
79257be
coordinate fixed range from data
13bzhang May 22, 2015
955da5e
Merge branch 'master' of https://github.com/ropensci/plotly into baob…
13bzhang May 22, 2015
2dc1453
delete an extra line in test
13bzhang May 22, 2015
7cefa02
Merge branch 'master' into baobao-equal_axes
13bzhang May 29, 2015
5d9deca
made test different from ggplotly
13bzhang May 29, 2015
b969dee
test expect_equal with tolerance
13bzhang May 30, 2015
af4fd33
added parameter name, made tolerance smaller
13bzhang Jun 2, 2015
b7f1645
Merge branch 'baobao-equal_axes' of https://github.com/ropensci/plotl…
cpsievert Dec 11, 2015
ad260ed
merge with master
cpsievert Dec 12, 2015
0a85c90
avoid using random data
cpsievert Dec 12, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions R/ggplotly.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,22 @@ gg2list <- function(p) {
stop("No exportable traces")
}

# fixed coordinates: if the coordinates ratio is not NULL, then
# we make the size of the plot according to the specified ratio
# note: we set the biggest dimension to 600
if (!is.null(p$coordinates$ratio)) {
x_range <- range(built[[2]]$ranges[[1]]$x.major_source, na.rm = TRUE)
y_range <- range(built[[2]]$ranges[[1]]$y.major_source, na.rm = TRUE)
yx_ratio <- (y_range[2] - y_range[1]) / (x_range[2] - x_range[1])
if (yx_ratio > 1) {
layout$height <- 600
layout$width <- layout$height * (1 / p$coordinates$ratio) * (1 / yx_ratio)
} else {
layout$width <- 600
layout$height <- layout$height * (1 / p$coordinates$ratio) * yx_ratio
}
Copy link
Collaborator

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

}

mode.mat <- matrix(NA, 3, 3)
rownames(mode.mat) <- colnames(mode.mat) <- c("markers", "lines", "none")
mode.mat["markers", "lines"] <-
Expand Down
52 changes: 52 additions & 0 deletions tests/testthat/test-ggplot-coord.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
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))
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure set.seed() ensures we have the same set of numbers, so image diffs should be meaningful. That being said, I prefer setting the seed over copy/pasting numbers (I think it makes the tests easier to read).

Even better, use a built-in dataset such as mtcars, cars, iris, diamonds, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. set.seed() ensures we get the same data each time. I second Carson's point. I used the set.seed with the same seed as used in the R Cookbook. But I can use one of the datasets that come with R if it's preferred.

Gotta go to graduation now! Will be back shortly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @cpsievert and @13bzhang ! I have learned something today. :)
I agree, set.seed(1); runif(100) is definitely easier to read (and diff) than a sequence of 100 numbers...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkcor : Does that mean we can keep set.seed(22) as it was written in the codebook?

Copy link
Contributor

Choose a reason for hiding this comment

The 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
built <- ggplot_build2(p)
x_range <- range(built[[2]]$ranges[[1]]$x.major_source, na.rm = TRUE)
y_range <- range(built[[2]]$ranges[[1]]$y.major_source, na.rm = TRUE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should get the ranges directly from the data instead of via ggplot_build()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the range of the data itself be different from the ggplot_build() range if people decide to set their own xlim and ylim? I wanted to account for potential issues with that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but you aren't setting xlim() or ylim() in the test itself, so you currently aren't testing for that...

In general, using the raw data as ground truth in tests is a good idea. Suppose ggplot2 internals change at some point and the x range is no longer stored in this location: built[[2]]$ranges[[1]]$x.major_source. This test wouldn't necessarily detect any problems since you're basically testing that we map the ggplot object in a certain way (which may be wrong).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Carson,
Good point. Ok, the changes should be made in ggplotly.R too righto?
@chriddyp @mkcor @chriddyp

yx_ratio <- (y_range[2] - y_range[1]) / (x_range[2] - x_range[1])
expect_identical(la$height/la$width, yx_ratio * p$coordinates$ratio)
})

# 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
built <- ggplot_build2(p)
x_range <- range(built[[2]]$ranges[[1]]$x.major_source, na.rm = TRUE)
y_range <- range(built[[2]]$ranges[[1]]$y.major_source, na.rm = TRUE)
yx_ratio <- (y_range[2] - y_range[1]) / (x_range[2] - x_range[1])
expect_identical(la$height/la$width, yx_ratio * p$coordinates$ratio)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@13bzhang Sorry, I must have clicked the wrong line: I meant "Please add new line at end of file" so we don't get this red sign from GitHub. Note that if you edit with vim, a new line at end of file is automatically added; in RStudio's editor, you need to actually add a line break after your last line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkcor
Got it! I added the space to the end of the test.