Allow parsing expressions in facet_wrap #656

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants
@wch
Collaborator

wch commented Sep 4, 2012

Fixes #25.

This should have visual tests before it's merged.

@wch

This comment has been minimized.

Show comment
Hide comment
@wch

wch Sep 4, 2012

Collaborator

Some useful examples:

m <- mpg
levels(m$drv) <- paste("Y[", 1:3, "]", sep="")
p <- ggplot(m, aes(x = displ, y = cty)) + geom_point()
p + facet_wrap(~drv, labeller = label_parsed)
p + facet_wrap(~drv+fl, labeller = label_parsed)
p + facet_wrap(~drv)
p + facet_wrap(~drv+fl)
p + facet_wrap(~drv+fl, labeller = label_both)
Collaborator

wch commented Sep 4, 2012

Some useful examples:

m <- mpg
levels(m$drv) <- paste("Y[", 1:3, "]", sep="")
p <- ggplot(m, aes(x = displ, y = cty)) + geom_point()
p + facet_wrap(~drv, labeller = label_parsed)
p + facet_wrap(~drv+fl, labeller = label_parsed)
p + facet_wrap(~drv)
p + facet_wrap(~drv+fl)
p + facet_wrap(~drv+fl, labeller = label_both)
@lselzer

This comment has been minimized.

Show comment
Hide comment
@lselzer

lselzer Sep 4, 2012

Contributor

That doesn't work with multiple factors because "," is not a possible plotmath expression. I fixed it but I'm not sure how submit it.

Contributor

lselzer commented Sep 4, 2012

That doesn't work with multiple factors because "," is not a possible plotmath expression. I fixed it but I'm not sure how submit it.

@wch

This comment has been minimized.

Show comment
Hide comment
@wch

wch Sep 4, 2012

Collaborator

See here:
https://github.com/hadley/ggplot2/wiki/Developing-ggplot2-using-github

Although it may be a bit more complicated since you're making a modification of my branch, instead of Hadley's master branch. If that ends up being too much trouble, you can just cut/paste the patch here.

Collaborator

wch commented Sep 4, 2012

See here:
https://github.com/hadley/ggplot2/wiki/Developing-ggplot2-using-github

Although it may be a bit more complicated since you're making a modification of my branch, instead of Hadley's master branch. If that ends up being too much trouble, you can just cut/paste the patch here.

@wch

This comment has been minimized.

Show comment
Hide comment
@wch

wch Sep 5, 2012

Collaborator

I think its better to use an expression list, which separates the expressions with commas - see my latest commit.

It will render the example you provided, with commas:

p + facet_wrap(~drv+fl, labeller = label_parsed)

It might be better to add a new labeler function to scales that makes a list automatically.

Collaborator

wch commented Sep 5, 2012

I think its better to use an expression list, which separates the expressions with commas - see my latest commit.

It will render the example you provided, with commas:

p + facet_wrap(~drv+fl, labeller = label_parsed)

It might be better to add a new labeler function to scales that makes a list automatically.

@wch

This comment has been minimized.

Show comment
Hide comment
@wch

wch Sep 5, 2012

Collaborator

The problem is in train_position, where it loops over calls to scale_apply, for each layer. In scale_apply, it calls scale_train.discrete, which uses the drop parameter of the scale object.

I think the way to do this is to first loop over all the layers with scale_train, then do the drop afterward. But doing that correctly could be tricky.

Collaborator

wch commented Sep 5, 2012

The problem is in train_position, where it loops over calls to scale_apply, for each layer. In scale_apply, it calls scale_train.discrete, which uses the drop parameter of the scale object.

I think the way to do this is to first loop over all the layers with scale_train, then do the drop afterward. But doing that correctly could be tricky.

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Sep 5, 2012

Member

The problem with this approach is that it doesn't allow for other labeller functions. To really fix this we need a more flexible specification of labeller functions - they should take a matrix (or other richer data structure) as an argument. But this will mean that we need to rewrite the existing labeller functions and hence is an API breaking change, which means we need to push it off to a future version.

Member

hadley commented Sep 5, 2012

The problem with this approach is that it doesn't allow for other labeller functions. To really fix this we need a more flexible specification of labeller functions - they should take a matrix (or other richer data structure) as an argument. But this will mean that we need to rewrite the existing labeller functions and hence is an API breaking change, which means we need to push it off to a future version.

@metasoarous

This comment has been minimized.

Show comment
Hide comment
@metasoarous

metasoarous Aug 2, 2013

This is making me sad... +1 for future version feature request.

Thanks!

This is making me sad... +1 for future version feature request.

Thanks!

@tiflo

This comment has been minimized.

Show comment
Hide comment
@tiflo

tiflo Aug 10, 2013

Same here. This doesn't seem to have been fixed. writing a (variable-specific) labeler function every time I'd like to relabel facets seems tedious. Sorry, if this has been fixed and I missed it. cheers.

tiflo commented Aug 10, 2013

Same here. This doesn't seem to have been fixed. writing a (variable-specific) labeler function every time I'd like to relabel facets seems tedious. Sorry, if this has been fixed and I missed it. cheers.

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Feb 24, 2014

Member

This sounds like a great feature, but unfortunately we don't currently have the development bandwidth to support it. If you'd like to submit a pull request that implements this feature, please follow the instructions in the development vignette.

Member

hadley commented Feb 24, 2014

This sounds like a great feature, but unfortunately we don't currently have the development bandwidth to support it. If you'd like to submit a pull request that implements this feature, please follow the instructions in the development vignette.

@hadley hadley closed this Feb 24, 2014

@lionel-

This comment has been minimized.

Show comment
Hide comment
@lionel-

lionel- Jun 19, 2014

Member

Hello,

I tried Winston's patch and it seems fully functional, on par with facet_grid's labeller.
Can I ask what was wrong? If there is a problem I'd like to try to solve it.

Member

lionel- commented Jun 19, 2014

Hello,

I tried Winston's patch and it seems fully functional, on par with facet_grid's labeller.
Can I ask what was wrong? If there is a problem I'd like to try to solve it.

@lionel-

This comment has been minimized.

Show comment
Hide comment
@lionel-

lionel- Jun 13, 2015

Member

@hadley what was the problem with this labeller exactly? last time I tried it it seemed to work fine. If it works in most situations, maybe it'd still be nice to have it with informative error messages when it doesn't work.

Member

lionel- commented Jun 13, 2015

@hadley what was the problem with this labeller exactly? last time I tried it it seemed to work fine. If it works in most situations, maybe it'd still be nice to have it with informative error messages when it doesn't work.

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Jun 13, 2015

Member

@lionel- I don't remember. If you want to do a fresh PR now is a good time.

Member

hadley commented Jun 13, 2015

@lionel- I don't remember. If you want to do a fresh PR now is a good time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment