Skip to content

Add stat_ecdf support #2052

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

moutikabdessabour
Copy link
Contributor

@moutikabdessabour moutikabdessabour commented Oct 22, 2021

  • change NEWS.md

@moutikabdessabour moutikabdessabour linked an issue Oct 22, 2021 that may be closed by this pull request
Comment on lines 626 to 627
to_basic.default<- function(data, prestats_data, layout, params, p, ...) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
to_basic.default<- function(data, prestats_data, layout, params, p, ...) {
to_basic.default <- function(data, prestats_data, layout, params, p, ...) {

@@ -252,7 +252,7 @@ to_basic.GeomLine <- function(data, prestats_data, layout, params, p, ...) {

#' @export
to_basic.GeomStep <- function(data, prestats_data, layout, params, p, ...) {
prefix_class(data, "GeomPath")
prefix_class(if((params$direction %||% "vh") %in% c("vh", "hv", "mid")) ggplot2:::stairstep(data, direction = params$direction) else data, c("GeomPath", "GeomStep"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using ::: isn't allowed by R CMD check. Use ggfun() instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason this breaks default geom_step() usage. See, for example, the change in tests/testthat/_snaps/ggplot-step/step-gg-hv.svg

Copy link
Contributor Author

@moutikabdessabour moutikabdessabour Nov 2, 2021

Choose a reason for hiding this comment

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

I think I fixed the test issues. basically just re rendering the svg solved it

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, the tests are still broken. I don't think this is the right fix.

@cpsievert
Copy link
Collaborator

cpsievert commented Nov 2, 2021

This isn't the right approach, I'll take this on in #2065

@cpsievert cpsievert closed this Nov 2, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stat_ecdf does not work
2 participants