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

replace use of pl$polars_set_options() with pl$options$... in docs #316

Closed
sorhawell opened this issue Jul 8, 2023 · 8 comments
Closed
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@sorhawell
Copy link
Collaborator

pl$options supports auto completion and make it easier to discover options

image
@sorhawell sorhawell added enhancement New feature or request good first issue Good for newcomers labels Jul 8, 2023
@Surya-Narayan
Copy link

Hello, I would like to work on this issue if its okay. I did try to find 'pl$polars_set_options()' in the docs directory but was not able to. Can you help me with what exactly is the requirement ?

@sorhawell
Copy link
Collaborator Author

Dear @Surya-Narayan . That sounds amazing :)

here are the current docs:

current use set/get_polars_options
https://github.com/search?q=repo%3Apola-rs%2Fr-polars%20set_polars_options&type=code
https://github.com/search?q=repo%3Apola-rs%2Fr-polars%20get_polars_options&type=code

replace pl$set_polars_options(no_messages=TRUE) with pl$options$no_messages(TRUE)
and likewise use pl$options$no_messages() instead of get.

You should fork r-polars main. Then make a new branch called e.g. options_docs and work there. You can fairly fast make a draft PR back to r-polars to see if tests are passing, and we will take it from there.

@Surya-Narayan
Copy link

Okay got it. You can assign this to me! I am changing the set_polars_options in examples and I found these oddities :

image

How do I approach these ?

@etiennebacher
Copy link
Collaborator

@sorhawell I'm not entirely convinced by this change, it would feel a bit weird to do:

pl$options$strictly_immutable(TRUE)
pl$options$debug_polars(TRUE)

I think it's good to have pl$options to get a named list of options that is easy to use (which is currently the case), and pl$set_options() to set options like this:

pl$set_options(strictly_immutable = TRUE, debug_polars = TRUE)

This still needs some renaming from set_polars_options() to set_options() (the pl$ prefix already indicates that this is for polars only). @Surya-Narayan are you still interested in making this renaming?

Btw, I think we could add a global_string_cache item in pl$options.

@sorhawell
Copy link
Collaborator Author

sorhawell commented Sep 11, 2023

I personally was never happy with the Sys.setenv(...) and options(...) interfaces in R, as the users have to know in advance of the options and have to anyways look up the exact spelling, ... likely does not provide any auto-completion.

I wanted to have something with was faster and easier to use interactively.

Ideally I wanted to have some pl$options object which behaved as an environment(singleton list) with S3 methods $ $<- defined and still checking inserted values where correct. I failed to get that behavior right, and ended in some environment pointer loop.

As a second choice, I went with same interface type as shiny reactiveVal in respect to that pl$options$some_option() is read and pl$options$some_option(some_new_opt_val) is write. Maybe that is not too familiar to R users.

@etiennebacher If going along with your suggestion. Maybe every option arg in pl$set_options() could be named and defaulted to NULL, where NULL is aliased with no change. We can model unset as NA or "".

then it could be
pl$set_options(strictly_immutable = NULL, debug_polars = NULL, , , ,)

then auto completion and "doc strings" would work well with e.g. Rstudio (plot as example)
image

Btw, I think we could add a global_string_cache item in pl$options.

We have some rust-side option statefulness which could be added such as
pl$set_global_rpool_cap() and pl$get_global_rpool_cap. The state of global_rpool_cap is kept on rust side to preserve strict thread safety, as it is accessed by multiple threads. We could make get_set_global_rpool_cap private and read/write the state via pl$options$rpool_cap pl$set_options(rpool_cap = 42) such that it in any ways appears to be an R side state.

The global_string_cache-stuff is also a rust side state 'deep' into rust-polars. It is confusing because it is actually modeled as a "global reference counter" so it cannot accurately be represented as some option state such as TRUE/FALSE or N=0,1,2,3... on R side. This is also confusing for py-polars develops. Instead we could have some "verb"-options like pl$enable_string_cache() and pl$reset_string_cache() which are functions and still nudge user towards using pl$with_string_cache()

As side note: There is also an issue to refactor polars private options state representation polars:::polars_optenv (an R environment object) into plain environment variables. The benefit is most options can be set as such and it would simplify some option passing between R and rust.

@etiennebacher
Copy link
Collaborator

If going along with your suggestion. Maybe every option arg in pl$set_options() could be named and defaulted to NULL, where NULL is aliased with no change. We can model unset as NA or "".

Yes we should use named arguments and auto-completion would work. I just made a quick example with strictly_immutable:

image

Would you be ok with something like this? I don't see why we should set them to NULL though, why not put TRUE or FALSE as default value for each argument?

We could make get_set_global_rpool_cap private and read/write the state via pl$options$rpool_cap pl$set_options(rpool_cap = 42) such that it in any ways appears to be an R side state.

This would make sense to me, I think it's confusing to set some options with pl$set_polars_options() and others with pl$set_global_rpool_cap().

Instead we could have some "verb"-options like pl$enable_string_cache() and pl$reset_string_cache() which are functions and still nudge user towards using pl$with_string_cache()

There's a PR in rust-polars to fix the strange way to set string cache: pola-rs/polars#11020

@sorhawell
Copy link
Collaborator Author

sorhawell commented Sep 11, 2023

Would you be ok with something like this? I don't see why we should set them to NULL though, why not put TRUE or FALSE as default value for each argument?

it would force updating state of all options that are not NULL. Depending on underlying state representations in R and rust maybe that is less elegant. But probably OK.

However something like this would surprise a user, as strictly_immutable is reverted to its default (TRUE) by changing another unrelated option.

pl$set_options(strictly_immutable = FALSE)
pl$set_options(some_other_option = FALSE)

pl$options$strictly_immutable
> TRUE

@etiennebacher
Copy link
Collaborator

This is mostly outdated now, I'll open another issue about what to do with global_rpool_cap()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants