Skip to content

Makes the option to center the steps in geom_step() work #3435

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

Merged
merged 12 commits into from
Aug 6, 2019

Conversation

mastropi
Copy link
Contributor

@mastropi mastropi commented Jul 11, 2019

Fixes bug in PR#2365 which also closes issue #3348 where the new center option is requested.

The fix was done starting off from the forked repository by rmatkins where he had implemented the new feature requested in #3348.

The bug in PR#2365 was fixed by replacing the last line in the stairstep() function from:
data_frame(x, y, data_attr)
to:
new_data_frame(c(list(x=x, y=y), data_attr))

R. Matthew Atkins and others added 10 commits December 9, 2017 00:00
…tion. In this case, the "step" (or vertical deflection) occurs halfway between point A and B.
Merging with ggplot2 master on 5-20-18
Co-Authored-By: rmatkins <33901289+rmatkins@users.noreply.github.com>
Co-Authored-By: rmatkins <33901289+rmatkins@users.noreply.github.com>
Before the change the following error message was generated by the replaced line:
"Error: Elements must be named"
@mastropi mastropi changed the title Fixes bug in PR#2365 which also closes issue #3348 (geom_step) Makes the option to center the steps in geom_step() work Jul 12, 2019
@mastropi
Copy link
Contributor Author

Regarding the code coverage failures, it's not clear to me where I should add unit tests for the new functionality of geom_step() implemented in this PR...
I checked testthat/test-geom-path.R but I didn't find any tests on geom_step().

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this PR! It's very close, but needs a few changes before it can be merged. See the above comments, plus:

  • Could you remove randomX.png and randomY.png?
  • As you've noted, this PR needs tests. There are currently no tests for the stairstep() function, which is nicely isolated and I think could be tested quite readily. With proper testing of the stairstep() function, I don't think a visual test is needed. Something like the following in tests/testthat/test-geom-path.R would work nicely:
test_that("stairstep() errors with too few observations", {

})

test_that("stairstep() output is correct for direction = 'vh'", {
  df <- data_frame(x = 1:3, y = 1:3)
  stepped <- stairstep(df, direction = "vh")
})

test_that("stairstep() output is correct for direction = 'hv'", {
  df <- data_frame(x = 1:3, y = 1:3)
  stepped <- stairstep(df, direction = "hv")
})

test_that("stairstep() output is correct for direction = 'mid'", {
  df <- data_frame(x = 1:3, y = 1:3)
  stepped <- stairstep(df, direction = "mid")
})

@willfischer
Copy link

Looking to use this exact functionality for some production code. Is it near release?

@mastropi
Copy link
Contributor Author

I intend to perform the suggested changes and write the pending unit tests by Sunday.

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

These tests are really good... thanks for adding them! Just two details and then it looks good to merge!

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thanks!

@paleolimbot paleolimbot merged commit 541ae99 into tidyverse:master Aug 6, 2019
@lock
Copy link

lock bot commented Feb 2, 2020

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Feb 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants