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

Wrong Unittests #199

Closed
jankatins opened this issue Feb 6, 2014 · 4 comments
Closed

Wrong Unittests #199

jankatins opened this issue Feb 6, 2014 · 4 comments

Comments

@jankatins
Copy link
Contributor

Doing some refactoring work for #188 brought up some interesting "bad" unittests: I changed the tranformation and get_layer code so that it errors if a mapping in aes is used to set a value. From my POW, thats wrong as one can see from the below differences between ggplot2 and our current implementation:

from ggplot.tests.test_basic.test_alpha_density, which now fails

def test_alpha_density():
    gg = ggplot(aes(x='mpg', fill=True, alpha=0.3), data=mtcars)
    assert_same_ggplot(gg + geom_density(), "geom_density_alpha")

geom_density_alpha

vs ggplot2:

library(ggplot2)
gg = ggplot(aes(x=mpg, fill=TRUE, alpha=0.3), data=mtcars)
gg + geom_density()

rplot

@jankatins
Copy link
Contributor Author

I've now changed all unittests to make the setting in the geom_* call without aes(...).

Whats still to be done is to implement fill: currently it can be set to "True", which is not a valied color in ggplot2:

> gg = ggplot(aes(x=mpg,), data=mtcars)
> gg + geom_density(fill=TRUE, alpha=0.3)
Error in col2rgb(colour, TRUE) : ungültiger Farbname in 'TRUE'

You also cannot set different colors than blue.

The problems should be handeled in #191

@jankatins
Copy link
Contributor Author

What's also open in this issue, is the question whether we want to support setting a mapping to a value: ggplot supports it but it has the funny effect that ggplot interprets it as a discrete variable where all values are the same. I will make some changes to my new code to do the same (silly) behaviour.

@has2k1
Copy link

has2k1 commented Mar 27, 2014

Currently we accept fill = True as a manual aesthetic to mean "yes do fill". It seems rather clear what the user wants. In #252 I have kept this behaviour and the fill value used is the same as the colour of the line.

The only issue is, geom_density(aes(fill=True)) and geom_density(fill=True) would do the filling with different colours. It would have made more sense to be strict with the mappings and less so with the manual aesthetics.

@glamp
Copy link
Contributor

glamp commented May 31, 2016

no longer relevant

@glamp glamp closed this as completed May 31, 2016
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

No branches or pull requests

3 participants