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

subset_draws duplicates variables if variables argument contains NA #365

Open
n-kall opened this issue May 8, 2024 · 4 comments
Open

subset_draws duplicates variables if variables argument contains NA #365

n-kall opened this issue May 8, 2024 · 4 comments

Comments

@n-kall
Copy link
Collaborator

n-kall commented May 8, 2024

When subsetting by variable, if NA is included in the variable argument along with other strings, the matched variables are duplicated in the output.

Example:

example_draws() |>
subset_draws(variable = c("mu", NA)) |>
variables()

#> [1] "mu" "mu"
@mjskay
Copy link
Collaborator

mjskay commented May 8, 2024

Looks like an issue with the check_existing_variables() helper:

(check_existing_variables(c("mu", NA), example_draws()))
## [1] "mu" "mu"

@n-kall
Copy link
Collaborator Author

n-kall commented May 8, 2024

Great, probably not too complex of a fix then. But I also wonder if it ever makes sense to allow NA in variable. Maybe there should be an input check disallowing this?

@mjskay
Copy link
Collaborator

mjskay commented May 8, 2024

Yeah good point. The two options to my mind are:

  • throw an error in check_existing_variables() if any elements of variable are NA.
  • something consistent with how variable selection works in base R subsetting, like returning a variable that is ndraws(x) of NA for that entry in the output.

I think I'd be fine with throwing an error since it probably isn't that useful to do variable subsetting by names with NAs, and throwing an error would be simpler. Then if a use case arises for this later we could consider implementing it.

@n-kall
Copy link
Collaborator Author

n-kall commented Jun 4, 2024

@mjskay I think your first suggestion (throwing an error if names includes NA) would make the most sense at this stage

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

No branches or pull requests

2 participants