fixed problem with equal axes #223
Conversation
|
@13bzhang looking good! I forgot to mention that when we add a new feature, we should add a test to demonstrate that it's working. In this case, I think starting a |
|
Cool, I will add a test. |
|
I added a testthat test as Carson suggested. |
|
Looks like the new tests aren't passing. There should be a little icon next to each commit to signify whether or not the build was successful. I'm having the same problem over at #217. Not sure why that is. |
|
Hi Carson, Sorry I'm stuck going through commencement weekend. I will take a look at this today. |
|
|
||
| # 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.
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.
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.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
@mkcor : Does that mean we can keep set.seed(22) as it was written in the codebook?
|
@cpsievert |
On TravisCI, commit 31a1ef6 was successfully merged with 789e1ec (master) to create 147d8a4. A visual testing table comparing 789e1ec with 147d8a4 can be found here: |
| 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) | ||
| }) |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@mkcor
Got it! I added the space to the end of the test.
On TravisCI, commit ba738b9 was successfully merged with 789e1ec (master) to create 62801e5. A visual testing table comparing 789e1ec with 62801e5 can be found here: |
|
@cpsievert @mkcor @chriddyp |
On TravisCI, commit 23eb47b was successfully merged with 789e1ec (master) to create ff6f23f. A visual testing table comparing 789e1ec with ff6f23f can be found here: |
| # 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) |
There was a problem hiding this comment.
Maybe we should get the ranges directly from the data instead of via ggplot_build()?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
13bzhang
force-pushed
the
baobao-equal_axes
branch
from
23eb47b to
955da5e
Compare
|
Hi Carson, I changed the range to get it from the data instead of from |
|
@cpsievert 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_identical(la$height/la$width, round(yx_ratio) * p$coordinates$ratio)
})The range ratio from the raw data is going to be a bit off from the |
On TravisCI, commit 5d9deca was successfully merged with fb40491 (master) to create a5355cd. A visual testing table comparing fb40491 with a5355cd can be found here: |
|
No worries @13bzhang, thanks for making those changes! I think |
|
Thanks for the tip. I tried it with |
|
@cpsievert I added the parameter name |
On TravisCI, commit af4fd33 was successfully merged with fb40491 (master) to create 357c7c3. A visual testing table comparing fb40491 with 357c7c3 can be found here: |
|
Ah, shoot, the test table didn't build since there were two builds happening at once. @13bzhang do you see the little "restart build" trigger on travis (for some reason I can't)? If so, please restart the build. |
|
I don't think I have access to the little restart build button because I
don't have enough access or whatever. I did a little hack where I closed
the PR and reopened it, which should trigger a rebuild.
|
|
Here she blows! 🐳 |
…into baobao-equal_axes
| } else { | ||
| layout$width <- 600 | ||
| layout$height <- layout$height * (1 / p$coordinates$ratio) * yx_ratio | ||
| } |
There was a problem hiding this comment.
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
|
fixed axes were added in f9e3f2a |
A solution to Issue #218 "Equal scaling of axes not working" -- so that the height and width ratios will be those specified by users. Conformed to the style guide.