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

Add arguments to $profile() #429

Merged
merged 20 commits into from
Oct 25, 2023
Merged

Add arguments to $profile() #429

merged 20 commits into from
Oct 25, 2023

Conversation

etiennebacher
Copy link
Collaborator

@etiennebacher etiennebacher commented Oct 18, 2023

  • Add all optimization parameters
  • Add arg show_plot

TODO:

  • more plot-related args: figsize, truncate_nodes
  • bump news

We can't test this with snapshots because the timing changes at every run. I used ggplot2 because doing the same graph with base R is just a pain, but if one of you wants to replace it with base R I don't have a problem with that.


Example:

library(polars)

countries = c(
  "France", "Germany", "United Kingdom", "Japan", "Columbia",
  "South Korea", "Vietnam", "South Africa", "Senegal", "Iran"
)

set.seed(123)
df_test = pl$LazyFrame(
  country = sample(countries, 1e7, TRUE),
  x = sample(1:100, 1e7, TRUE),
  y = sample(1:1000, 1e7, TRUE)
)$
  with_columns(contains_na = pl$col("country")$str$contains("na"))$
  sort(pl$col("country"))$
  filter(pl$col("country")$is_in(pl$lit(c("United Kingdom", "Japan", "Vietnam")))    )$
  with_columns(is_uk = pl$col("country") == "United Kingdom")

x <- df_test$profile(show_plot = TRUE)

x <- df_test$profile(show_plot = TRUE, truncate_nodes = 5)

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is an interesting feature.
While there are other packages out there for creating great plots besides ggplot2, it makes sense to go with ggplot2 here given that it is one of the most popular R package.

Some questions:

  • Would it make sense to add an option feature that would allow the display of the plot to be the default?
  • I think it might be useful if the ggplot object is the return value, is it possible to do that?

R/lazyframe__lazy.R Outdated Show resolved Hide resolved
@etiennebacher
Copy link
Collaborator Author

Would it make sense to add an option feature that would allow the display of the plot to be the default?

I don't really see the point here

I think it might be useful if the ggplot object is the return value, is it possible to do that?

We should return a list of 3 with the two tables + the plot when show_plot = TRUE, I'll update the function

@eitsupi
Copy link
Collaborator

eitsupi commented Oct 20, 2023

I don't really see the point here

Sorry for the lack of clarity. I mean, I was imagining something like specifying it with the options function.

@eitsupi
Copy link
Collaborator

eitsupi commented Oct 20, 2023

We should return a list of 3 with the two tables + the plot when show_plot = TRUE, I'll update the function

Nice!

Could you also update the document?

@etiennebacher
Copy link
Collaborator Author

Could you also update the document?

I added details on the returned value

I was imagining something like specifying it with the options function.

I don't think this is necessary, this function shouldn't be called many times so that we need a global option for this

R/lazyframe__lazy.R Outdated Show resolved Hide resolved
etiennebacher and others added 4 commits October 24, 2023 10:30
Merge branch 'main' into profile-plot
# Veuillez entrer un message de validation pour expliquer en quoi cette fusion est
# nécessaire, surtout si cela fusionne une branche amont mise à jour dans une branche de sujet.
#
# Les lignes commençant par '#' seront ignorées, et un message vide
# abandonne la validation.
Copy link
Collaborator

@sorhawell sorhawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :)

@etiennebacher etiennebacher merged commit bba633c into main Oct 25, 2023
10 checks passed
@etiennebacher etiennebacher deleted the profile-plot branch October 25, 2023 21:32
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 this pull request may close these issues.

None yet

3 participants