Skip to content

Conversation

@fukuhiro2023
Copy link
Collaborator

This PR fixes for multiple variables in observation, and add new argument filter_var in prepare_boxly() as part of it.

Copy link

Copilot AI left a 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_var parameter to prepare_boxly() function with default value "PARAM"
  • Refactored factor conversion logic to handle multiple observation variables using lapply()
  • Updated param_var assignment to use filter_var instead of obs_var for 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.

Comment on lines +122 to +130
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
}
})
Copy link

Copilot AI Dec 5, 2025

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.

Suggested change
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)))
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +116 to +117
stop(paste(
"The filter variable '", filter_var, "' is not found in the observation data.",
Copy link

Copilot AI Dec 5, 2025

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.

Suggested change
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. ",

Copilot uses AI. Check for mistakes.
@wangben718 wangben718 merged commit 1765036 into main Dec 10, 2025
13 checks passed
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.

A bug in prepare_boxly

3 participants