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

Better warning messages #14

Closed
spgarbet opened this issue Mar 13, 2023 · 7 comments
Closed

Better warning messages #14

spgarbet opened this issue Mar 13, 2023 · 7 comments
Assignees
Milestone

Comments

@spgarbet
Copy link
Member

spgarbet commented Mar 13, 2023

A user has reported the following warning messages and they would like to know the field names that are causing them.

Warning messages:
1: In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  wrong number of fields in entry(ies) 4298, 124804
2: In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  wrong number of fields in entry(ies) 4475, 9609, 18554
3: In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  wrong number of fields in entry(ies) 23167, 23907, 24892, 29784, 70058, 70915, 88322, 90332, 95099, 102932
4: In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  13 entries set to NA due to wrong number of fields
5: In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  19 entries set to NA due to wrong number of fields
6: In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  wrong number of fields in entry(ies) 23927, 23928, 70889, 135252, 135255
7: In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  wrong number of fields in entry(ies) 9865, 17464, 23926, 24910, 24911, 27861, 70889, 135255
8: In (function (nm, lab)  : Missing field for suffix arm_results
9: In (function (nm, lab)  : Missing field for suffix arm_results_esp
10: In (function (nm, lab)  : Missing field for suffix send_results___1

We need test cases added that create these warnings on export. Then capture the warnings somehow and modify with the field name causing them.

@spgarbet
Copy link
Member Author

Looking through the code, some places use suppressWarnings and maybe we could open those up and have better descriptive messages for those failures.

"integer" = suppressWarnings(as.numeric(records[[i]]))

This is difficult. A user would want a warning message if the column was defined on the form and required, but no warnings otherwise. Exposing this could create more issues than it solves.

@spgarbet
Copy link
Member Author

I can recreate the unpaste warnings like this:

> chron::times("12:30", format=c(times="h:m:s"))
Error in convert.times(times., fmt) : format h:m:s may be incorrect
In addition: Warning message:
In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  wrong number of fields in entry(ies) 1

@nutterb
Copy link
Collaborator

nutterb commented Mar 13, 2023

Looking through the code, some places use suppressWarnings and maybe we could open those up and have better descriptive messages for those failures.

"integer" = suppressWarnings(as.numeric(records[[i]]))

This is difficult. A user would want a warning message if the column was defined on the form and required, but no warnings otherwise. Exposing this could create more issues than it solves.

Context....

These suppressWarnings calls have been there since even before my time (initial commit). I would wager a guess that they were intended to avoid a warning from converting character values to numeric. I don't think R produces that many of these:

as.numeric(c("1.2", "", " ", NA))

as.numeric("NA")

I would guess the original intent of suppressWarnings was largely addressed by adding na.strings = "" to the read.csv call. You might not create as many issues getting rid of this as you think you might.

@spgarbet
Copy link
Member Author

Well we have some users who can test versus large databases, so I might do a test run against those.

@nutterb
Copy link
Collaborator

nutterb commented Mar 13, 2023

I can recreate the unpaste warnings like this:

> chron::times("12:30", format=c(times="h:m:s"))
Error in convert.times(times., fmt) : format h:m:s may be incorrect
In addition: Warning message:
In unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3) :
  wrong number of fields in entry(ies) 1

Theory: the field generating the error appears to be a "time" field. Are there any values that don't follow a strict hh:mm format? (perhaps missing a leading zero)?

x1 <- gsub("(^\\d{2}:\\d{2}$)", "\\1:00", "03:30")
chron::times(x1, format = c(times = "h:m:s"))
#> [1] 03:30:00


# NOTICE THE LACK OF A LEADING 0 HERE-----|
x2 <- gsub("(^\\d{2}:\\d{2}$)", "\\1:00", "3:30")
chron::times(x2, format = c(times = "h:m:s"))
#> Warning in unpaste(times, sep = fmt$sep, fnames = fmt$periods, nfields = 3):
#> wrong number of fields in entry(ies) 1
#> Error in convert.times(times., fmt): format h:m:s may be incorrect



# teaching the regex to recognize either one or two digits compensates
x3 <- gsub("(^\\d{1,2}:\\d{1,2}$)", "\\1:00", "3:30")
chron::times(x3, format = c(times = "h:m:s"))
#> [1] 03:30:00

@spgarbet
Copy link
Member Author

spgarbet commented Mar 13, 2023

The underlying issue is they allowed text entry for a time field, later changed it to time and there's borked data in their REDCap. However, there are hundreds of fields and they just need to know which one. The jist of this request is better warning messages.

@spgarbet spgarbet self-assigned this Mar 15, 2023
@spgarbet
Copy link
Member Author

I'm picking this one up. I know what to do.

@spgarbet spgarbet added this to the 2.5.1 milestone Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants