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

Unexpected failure of density plot without any error or warning #3250

Closed
clauswilke opened this issue Apr 17, 2019 · 4 comments · Fixed by #4866
Closed

Unexpected failure of density plot without any error or warning #3250

clauswilke opened this issue Apr 17, 2019 · 4 comments · Fixed by #4866
Assignees
Milestone

Comments

@clauswilke
Copy link
Member

I encountered the following example when teaching density plots. If a discrete variable is encoded as numerical (e.g., 0/1), and that variable is mapped onto fill, the variable values get replaced with NA and no fill is applied/no grouping is calculated. Importantly, this happens without any kind of error or warning message, so students get no indication of what they have done wrong, or that something went wrong at all.

It's not immediately clear to me whether and how we can catch this situation, but it would be good if we could. Reprex follows.

library(ggplot2)

# desired plot
p1 <- ggplot(mtcars, aes(mpg, fill = factor(am))) + geom_density(alpha = 0.4)
p1

# broken plot because factor() is missing
p2 <- ggplot(mtcars, aes(mpg, fill = am)) + geom_density(alpha = 0.4)
p2

# fill has been set to NA
head(layer_data(p2))
#>            y PANEL        x    density    scaled  ndensity     count  n
#> 1 0.01922476     1 10.40000 0.01922476 0.2829075 0.2829075 0.6151923 32
#> 2 0.01951519     1 10.44599 0.01951519 0.2871815 0.2871815 0.6244861 32
#> 3 0.01980689     1 10.49198 0.01980689 0.2914740 0.2914740 0.6338204 32
#> 4 0.02010390     1 10.53796 0.02010390 0.2958448 0.2958448 0.6433248 32
#> 5 0.02040182     1 10.58395 0.02040182 0.3002289 0.3002289 0.6528583 32
#> 6 0.02070509     1 10.62994 0.02070509 0.3046918 0.3046918 0.6625630 32
#>   group ymin       ymax fill weight colour alpha size linetype
#> 1    -1    0 0.01922476   NA      1  black   0.4  0.5        1
#> 2    -1    0 0.01951519   NA      1  black   0.4  0.5        1
#> 3    -1    0 0.01980689   NA      1  black   0.4  0.5        1
#> 4    -1    0 0.02010390   NA      1  black   0.4  0.5        1
#> 5    -1    0 0.02040182   NA      1  black   0.4  0.5        1
#> 6    -1    0 0.02070509   NA      1  black   0.4  0.5        1

Created on 2019-04-17 by the reprex package (v0.2.1)

@clauswilke
Copy link
Member Author

I believe the problematic code lives here:

ggplot2/R/stat-.r

Lines 109 to 110 in 8359806

unique <- uniquecols(old)
missing <- !(names(unique) %in% names(new))

Non-unique data columns are silently dropped. We could issue a warning for non-unique missing columns. I just don't know how often such a warning might flag cases that are actually correct.

@clauswilke
Copy link
Member Author

@thomasp85 Since we're considering cleanup and breaking changes, I would bring up this issue once more, which is an eternal thorn in my side. It comes up every year when I teach ggplot2. I made a draft PR a long time ago that I could revive.

Since all this code does is create a warning, it's not really going to break anything. But, there's the risk of false-positive warnings. The way to avoid false positives is to allow geoms to indicate that the dropped columns were dropped on purpose, and I believe 2-3 geoms in ggplot2 itself would require this modification to avoid spurious warnings.

@thomasp85
Copy link
Member

Yeah, let's revive it and have a look... maybe the best thing is simply to ship the warning and get feedback from users as to when it's a false positive so we can tune it after the fact

@clauswilke
Copy link
Member Author

Ok. Will try to get this done next week.

@clauswilke clauswilke self-assigned this May 20, 2022
clauswilke added a commit to wilkelab/ggplot2_archive that referenced this issue Jun 9, 2022
clauswilke added a commit that referenced this issue Jul 7, 2022
* Check for dropped aesthetics and issue a warning if aesthetics are dropped that shouldn't be. Fixes #3250.

* update documentation
@clauswilke clauswilke removed the wip work in progress label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants