-
Notifications
You must be signed in to change notification settings - Fork 2
Fix bug for multiple variables in observation #46
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a bug related to handling multiple variables in observation data by introducing a new filter_var parameter to the prepare_boxly() function. The fix allows users to specify which variable should be used for filtering, with "PARAM" as the default value, decoupling it from the observation variable specification.
Key changes:
- Added
filter_varparameter toprepare_boxly()function with default value "PARAM" - Refactored factor conversion logic to handle multiple observation variables using
lapply() - Updated
param_varassignment to usefilter_varinstead ofobs_varfor proper filtering - Code formatting improvements in
meta_boxly()function
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| R/prepare_boxly.R | Added filter_var parameter with validation, refactored factor conversion for multiple variables, updated param_var assignment |
| man/prepare_boxly.Rd | Documentation update for new filter_var parameter |
| R/meta_boxly.R | Code formatting improvements (indentation and brace placement) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| obs[, obs_var] <- lapply(obs_var, function(var) { | ||
| x <- obs[[var]] | ||
| if (!is.factor(x)) { | ||
| message("In observation level data, the facet variable '", var, "' is automatically transformed into a factor.") | ||
| factor(x, levels = sort(unique(x))) | ||
| } else { | ||
| x | ||
| } | ||
| }) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assignment obs[, obs_var] <- lapply(...) will create a list column in the data frame instead of properly updating the individual columns. This is because lapply() returns a list, and assigning a list to multiple columns creates a single list-column.
To fix this, use a loop to assign each column individually:
for (var in obs_var) {
x <- obs[[var]]
if (!is.factor(x)) {
message("In observation level data, the facet variable '", var, "' is automatically transformed into a factor.")
obs[[var]] <- factor(x, levels = sort(unique(x)))
}
}Alternatively, use obs[obs_var] <- lapply(...) (without the comma) to properly assign the list elements to respective columns.
| obs[, obs_var] <- lapply(obs_var, function(var) { | |
| x <- obs[[var]] | |
| if (!is.factor(x)) { | |
| message("In observation level data, the facet variable '", var, "' is automatically transformed into a factor.") | |
| factor(x, levels = sort(unique(x))) | |
| } else { | |
| x | |
| } | |
| }) | |
| for (var in obs_var) { | |
| x <- obs[[var]] | |
| if (!is.factor(x)) { | |
| message("In observation level data, the facet variable '", var, "' is automatically transformed into a factor.") | |
| obs[[var]] <- factor(x, levels = sort(unique(x))) | |
| } | |
| } |
| stop(paste( | ||
| "The filter variable '", filter_var, "' is not found in the observation data.", |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message uses inconsistent spacing in the paste() function, resulting in extra spaces around the filter variable name in the output. Remove the trailing space after the filter_var placeholder:
stop(paste(
"The filter variable '", filter_var, "' is not found in the observation data.",
"Please check the metadata for observation and `filter_var`."
))Should be:
stop(paste0(
"The filter variable '", filter_var, "' is not found in the observation data. ",
"Please check the metadata for observation and `filter_var`."
))Using paste0() gives better control over spacing.
| stop(paste( | |
| "The filter variable '", filter_var, "' is not found in the observation data.", | |
| stop(paste0( | |
| "The filter variable '", filter_var, "' is not found in the observation data. ", |
This PR fixes for multiple variables in observation, and add new argument
filter_varinprepare_boxly()as part of it.