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

Paste no longer works in the test version #9207

Closed
rdstern opened this issue Oct 27, 2024 · 4 comments · Fixed by #9237
Closed

Paste no longer works in the test version #9207

rdstern opened this issue Oct 27, 2024 · 4 comments · Fixed by #9237
Assignees
Milestone

Comments

@rdstern
Copy link
Collaborator

rdstern commented Oct 27, 2024

@N-thony so-far-so-good on the test version.
I have been writing the help on your undo and it is working well.

Now I have found the first bug and that is that the paste operation no longer seems to work.

I tried in diamonds, and then survey, just copying 3 cells from a single column and pasting them lower down into the same column. By the way the warning may have to change, because (when it works) I assume and hope, when it works that it can now be undone>

image
Anyway now I get this error:

image

So there is a problem now in the data_book$paste_from_clipboard function to be fixed!

I know this gives you new work, but in a way I'm quite pleased - this is what our testing is all about!!!

@rdstern rdstern added this to the 0.7.23 milestone Oct 27, 2024
@N-thony
Copy link
Collaborator

N-thony commented Oct 28, 2024

@N-thony so-far-so-good on the test version. I have been writing the help on your undo and it is working well.

Now I have found the first bug and that is that the paste operation no longer seems to work.

I tried in diamonds, and then survey, just copying 3 cells from a single column and pasting them lower down into the same column. By the way the warning may have to change, because (when it works) I assume and hope, when it works that it can now be undone>

image Anyway now I get this error:

image

So there is a problem now in the data_book$paste_from_clipboard function to be fixed!

I know this gives you new work, but in a way I'm quite pleased - this is what our testing is all about!!!

@rdstern this bug was there for long. I check in version 0.7.17 and I get the same error. I guess @Patowhiz has worked on the paste_from_clipboard function. Can you have a look?
image

@N-thony
Copy link
Collaborator

N-thony commented Oct 28, 2024

@Patowhiz I did a quick check on chatgpt and the issue arises in the loop where you're trying to replace values in the selected columns. Specifically, when you assign new_values <- clip_tbl[, index], new_values is a vector, and if you later use it directly in a conditional check, it will lead to the mentioned error. If you agree with code below, then someone in the team can update it?

Reads passed clipboard data and saves it to the selected data frame

DataSheet$set("public", "paste_from_clipboard", function(col_names, start_row_pos = 1, first_clip_row_is_header = FALSE, clip_board_text) {

Get the clipboard text contents as a data frame

clip_tbl <- clipr::read_clip_tbl(x = clip_board_text, header = first_clip_row_is_header)

Get the selected data frame

current_tbl <- self$get_data_frame(use_current_filter = FALSE)

Check if copied data rows are more than current data rows

if (nrow(clip_tbl) > nrow(current_tbl)) {
stop(paste("Rows copied cannot be more than number of rows in the data frame.",
"Current data frame rows:", nrow(current_tbl), ". Copied rows:", nrow(clip_tbl)))
}

If column names are missing, then just add the clip data as new columns and quit function

if (missing(col_names)) {
# Append missing values if rows are less than the selected data frame.
# New column rows should be equal to existing column rows
if (nrow(clip_tbl) < nrow(current_tbl)) {
empty_values_df <- data.frame(data = matrix(data = NA, nrow = (nrow(current_tbl) - nrow(clip_tbl)), ncol = ncol(clip_tbl)))
names(empty_values_df) <- names(clip_tbl)
clip_tbl <- rbind(clip_tbl, empty_values_df)
}
new_col_names <- colnames(clip_tbl)
for (index in seq_along(new_col_names)) {
self$add_columns_to_data(col_name = new_col_names[index], col_data = clip_tbl[, index])
}
return()
}

For existing column names

Check if the number of copied columns and selected columns are equal

if (ncol(clip_tbl) != length(col_names)) {
stop(paste("Number of columns are not the same.",
"Selected columns:", length(col_names), ". Copied columns:", ncol(clip_tbl)))
}

Check copied data integrity

for (index in seq_along(col_names)) {
col_data <- current_tbl[, col_names[index]]
# Get column type of column from the current table using column name
col_type <- class(col_data)
# Check copied data integrity based on the data type expected
if (is.factor(col_data)) {
# Get all the factor levels of the selected column in the current data frame
expected_factor_levels <- levels(col_data)
# Check if all copied data values are contained in the factor levels
# If any invalid is found, exit function
for (val in clip_tbl[, index]) {
if (!is.na(val) && !is.element(val, expected_factor_levels)) {
stop("Invalid column values. Level not found in factor")
}
} # End inner for loop
} else if (!(is.numeric(col_data) || is.logical(col_data) || is.character(col_data))) {
# clipr support above column types only. So pasting to a column not recognized by clipr may result to unpredictable results
# If not in any of the above column types then exit function
stop(paste("Cannot paste into columns of type:", col_type))
} # End if
} # End outer for loop

Replace values in the selected columns

for (index in seq_along(col_names)) {
# Set the row positions and the values
rows_to_replace <- start_row_pos:(start_row_pos + nrow(clip_tbl) - 1)
new_values <- clip_tbl[, index]

# Replace the old values with new values
for (i in seq_along(new_values)) {
  # Replace each value one by one
  self$replace_value_in_data(col_names = col_names[index], rows = rows_to_replace[i], new_value = new_values[i])
}

# Rename header if the first row of clip data is header
if (first_clip_row_is_header) {
  self$rename_column_in_data(curr_col_name = col_names[index], new_col_name = colnames(clip_tbl)[index]) 
}

} # End for loop
}) # End function

@rdstern
Copy link
Collaborator Author

rdstern commented Nov 2, 2024

@Vitalis95 has a lot on his plat just now. He is preparing tables stuff for @Patowhiz as well as finalising the 2/3 way summaries. I suggest @Patowhiz should be able to fix this quickly as he wrote it originally.
I am sending another example from Micah. It pastes easily into Excel, and it also pastes into our script window. Let's make it paste into the new data frame. This is now an option from the new front menu.

@Patowhiz
Copy link
Contributor

Patowhiz commented Nov 6, 2024

@N-thony @rdstern, thank you for raising this issue.

@rdstern, in summary, this error is due to the R upgrade, which causes certain older code implementations to throw errors. As I've mentioned before, R and its packages should be carefully evaluated with each upgrade, including reviewing documentation for any breaking changes. I hope the detailed explanations below demonstrate a balanced approach, encouraging both caution and progress when it comes to R-Instat development.

I recommend a thorough evaluation of this issue, as I am uncertain about how other parts of our R code base might be affected.

@N-thony, the lines in question were last worked on in 2022 and merged (after testing) into version 0.7.6, the last stable release. In that version, this functionality worked, so I was surprised it stopped functioning.

@rdstern and @N-thony, I suggest cross-referencing recent bugs with version 0.7.6, which underwent strict reviews, with minimal bugs and regressions. This is the version I revert to whenever I encounter issues with recent changes.

@N-thony, your ChatGPT response is partially correct, and its solution will resolve the problem. However, it misses the broader context, likely due to the information it was given. I hope this comment helps clarify that broader issue.

As I've emphasised before, debugging with RStudio saves time. I often use it to troubleshoot R-related issues, helping other developers by pinpointing the exact problems. As shown in the screenshot below, this bug originates from the self$replace_value_in_data function, not paste_from_clipboard. When I implemented paste_from_clipboard, I considered how self$replace_value_in_data was structured, which led me to omit the loop. Note code line 980 of the data_object_R6.R file, which allows new_value to contain multiple values. Despite the self$replace_value_in_data function name, the parameters and implementation suggest it can handle multiple replacements.

error_paste

Within self$replace_value_in_data, there are multiple calls to is.na(). As explained here, R 4.2.0 changes make if() or while() statements with conditions longer than one element throw an error rather than a warning. Previously, in R 4.1.3 (when @rdstern and I last tested this), these cases only triggered warnings. In the current R 4.4.1, the error get's thrown because self$paste_from_clipboard calls self$replace_value_in_data with multiple values.

message

I’m unsure if there are other parts of our code that use is.na in conditions where anyNA might be safer. It would be wise to check for potential bugs. I’ve now adjusted self$paste_from_clipboard to assume self$replace_value_in_data only accepts single values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants