wilkox / treemapify

🌳 Draw treemaps in ggplot2
http://wilkox.org/treemapify
213 stars 18 forks source link

Passing variables to subgroup doesn't work #25

Closed GregorDeCillia closed 6 years ago

GregorDeCillia commented 6 years ago

I currently develop an R package depending on treemaplify and some functionality broke due to changes in f29b1439c69ffa7a84b3a054d19590f1c30895e0. Now I'm trying to rewrite the functionality with the new (group, subgroup) interface and ran into an issue which seems like a bug to me. When I save a column to a string variable and pass that variable to the subgroup argument of treemapify, I get an error

treemapify(mtcars, area = "wt", subgroup = "am")
#> expected output of size 32 x 14

am_string <- "am"
treemapify(mtcars, area = "wt", subgroup = am_string)
#> Error in `[.default`(data, subgroups[[1]]) : 
#>   invalid subscript type 'symbol'
wilkox commented 6 years ago

I'm sorry about your package breaking – can you give me more details on the problem? I had intended for this release to be backwards-compatible.

I've pushed a fix for that bug to the dev version, you can install it with devtools::install_github('wilkox/treemapify').

GregorDeCillia commented 6 years ago

I was using the following call

dat <- treemapify(dat, area=x@area, fill=x@fill, label=x@key, group=x@gruppe)

where x is some S4 class capturing plot parameters. I totally don't mind reworking my code since the subgroup functionality seems like a very handy addition.

wilkox commented 6 years ago

Just pushed a dev release (58e6650, v2.5.0.9001) that restores fill and label as deprecated arguments, and group as an alias for subgroup. Would this fix the issue? I'll probably publish it to CRAN soon regardless to fix backwards compatibility for anyone who was calling against those arguments.

GregorDeCillia commented 6 years ago

Yes, the fix you provided in 899518c worked out well. As I said, I don't mind the interface changes and that was also not the reason I filed this issue - just the fact that I couldn't pass variables.

I've already adapted my project to an extend where I don't need the deprcated arguments anymore, but I can test them later today if you are interested.

wilkox commented 6 years ago

No need to test if you’re not going to use the changes, I just wanted to make sure there wasn’t another bug I didn’t know about. Thanks a lot for reporting this!

On 2 May 2018, at 22:13, GregorDeCillia notifications@github.com wrote:

Yes, the fix you provided in 899518c worked out well. As I said, I don't mind the interface changes and that was also not the reason I filed this issue - just the fact that I couldn't pass variables.

I've already adapted my project to an extend where I don't need the deprcated arguments anymore, but I can test them later today if you are interested.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.