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

Consider returning ggplot object instead of relying on print() #14

Closed
gadenbuie opened this issue Jul 23, 2019 · 2 comments
Closed

Consider returning ggplot object instead of relying on print() #14

gadenbuie opened this issue Jul 23, 2019 · 2 comments

Comments

@gadenbuie
Copy link

@gadenbuie gadenbuie commented Jul 23, 2019

Hi @alastairrushworth and thanks for this awesome package! I turn to it frequently to get a sense of new datasets.

One point of friction for me is that show_plot() doesn't return the ggplot2 object created by lower-level functions like plot_cat(). Currently, I believe that if type$method == "types" in show_plot() the result will be the ggplot2 object but otherwise, because of the if statements throughout show_plot(), the result will always be NULL.

library(dplyr)
library(inspectdf)

g <- starwars %>% 
  inspect_cat() %>% 
  show_plot()

g
#> NULL

This makes it difficult for users who would like to work with the ggplot2 object, to add or change styles, for example, because they need to fall back to using ::: to access inspectdf:::plot_cat() or similar. Unfortunately for these users, the default values for plot_cat() are handled by show_plot(), further increasing friction.

g2 <- starwars %>% 
  inspect_cat() %>% 
  inspectdf:::plot_cat()
#> Error in lapply(lvl_df$levels, merge_card, high_cardinality = high_cardinality): argument "high_cardinality" is missing, with no default

If I provide the default values to the lower level functions, then I can gain access to the created ggplot2 object, but it's clear that plot_cat() isn't designed for end user consumption.

g2 <- starwars %>% 
  inspect_cat() %>% 
  inspectdf:::plot_cat(
    df_names = list(df1 = "starwars"), 
    high_cardinality = 10, 
    col_palette = 0, 
    text_labels = TRUE, 
    label_thresh = 0.1
  )

g2
#> Warning: Stacking not well defined when not anchored on the axis

Personally, I would prefer that show_plot() simply return the ggplot2 object and that default printing rules are used to display the plot rather than explicitly calling print() internally. In this way, show_plot() would work as in the last example, but without automatically printing the plot. Doing this would give the user more control over where and how the inspectdf plots are used.

Created on 2019-07-23 by the reprex package (v0.2.1)

@alastairrushworth
Copy link
Owner

@alastairrushworth alastairrushworth commented Jul 24, 2019

Hi @gadenbuie thanks for the suggestion & the examples. This is a great point, I totally agree. This should be an easy fix, though I need to iron out some warnings that get emitted by printing some of the plots. I'll get back when I have an update.

Thanks!

@alastairrushworth
Copy link
Owner

@alastairrushworth alastairrushworth commented Jul 25, 2019

Hi @gadenbuie, this is now addressed by this commit. The next CRAN submission will follow shortly. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.