Skip to content
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

Inconsistent formals for StatIdentity$compute_layer #3202

Closed
coolbutuseless opened this issue Mar 24, 2019 · 4 comments
Closed

Inconsistent formals for StatIdentity$compute_layer #3202

coolbutuseless opened this issue Mar 24, 2019 · 4 comments
Milestone

Comments

@coolbutuseless
Copy link
Contributor

@coolbutuseless coolbutuseless commented Mar 24, 2019

The formals for compute_layer in the majority of the Stats in ggplot2 are c(self, data, params, layout)

Two Stats are different from the others: StatBindot and StatIdentity.

Is there a reason for the different formals for these 2 stats? Is this just a leftover from earlier version?

I'm certain the StatIdentity$compute_layer and StatBindot$compute_layer argument names don't matter (either passed through or ignored) but it may be worth fixing for consistency

# formals for `compute_layer` across all Stats
Stat                [1] "self"   "data"   "params" "layout"
StatBin             [1] "self"   "data"   "params" "layout"
StatBin2d           [1] "self"   "data"   "params" "layout"
StatBindot          [1] "self"   "data"   "params" "panels"
StatBinhex          [1] "self"   "data"   "params" "layout"
StatBoxplot         [1] "self"   "data"   "params" "layout"
StatContour         [1] "self"   "data"   "params" "layout"
StatCount           [1] "self"   "data"   "params" "layout"
StatDensity         [1] "self"   "data"   "params" "layout"
StatDensity2d       [1] "self"   "data"   "params" "layout"
StatEcdf            [1] "self"   "data"   "params" "layout"
StatEllipse         [1] "self"   "data"   "params" "layout"
StatFunction        [1] "self"   "data"   "params" "layout"
StatIdentity        [1] "data"   "scales" "params"
StatQq              [1] "self"   "data"   "params" "layout"
StatQqLine          [1] "self"   "data"   "params" "layout"
StatQuantile        [1] "self"   "data"   "params" "layout"
StatSf              [1] "self"   "data"   "params" "layout"
StatSfCoordinates   [1] "self"   "data"   "params" "layout"
StatSmooth          [1] "self"   "data"   "params" "layout"
StatSum             [1] "self"   "data"   "params" "layout"
StatSummary         [1] "self"   "data"   "params" "layout"
StatSummary2d       [1] "self"   "data"   "params" "layout"
StatSummaryBin      [1] "self"   "data"   "params" "layout"
StatSummaryHex      [1] "self"   "data"   "params" "layout"
StatUnique          [1] "self"   "data"   "params" "layout"
StatYdensity        [1] "self"   "data"   "params" "layout"
@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Mar 24, 2019

Thanks. I quickly checked the code.

Let me see if there are some historical reasons.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Mar 31, 2019

  • StatIdentity$compute_layer() was added by this commit: 66b9df7
  • StatBindot$compute_layer() was added by this commit: 09faf05

I found no special reason to keep them as is. It seems just because the common set of formals was data, scales, ... at that time (c.f. 8af983b#diff-2b0d9305b6b639cd9de20829bd3e66b2R63). So, probably we can fix them.

BTW,

Two Stats are different from the others

this is correct, but actually ggplot2 has only three compute_layer() methods.

$ git grep "compute_layer = "
R/position-.r:  compute_layer = function(self, data, params, layout) {
R/position-identity.r:  compute_layer = function(data, params, scales) {
R/position-jitter.r:  compute_layer = function(data, params, panel) {
R/position-nudge.R:  compute_layer = function(data, params, panel) {
R/stat-.r:  compute_layer = function(self, data, params, layout) {
R/stat-bindot.r:  compute_layer = function(self, data, params, panels) {
R/stat-identity.r:  compute_layer = function(data, scales, params) {

Oh, and positions might be good to fix as well.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 11, 2019

@yutannihilation can you make a PR for this?

@lock
Copy link

@lock lock bot commented Oct 21, 2019

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 Oct 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants