-
Notifications
You must be signed in to change notification settings - Fork 3
Add customizable sex coding to checkParentIDs #106
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
Introduced code_male and code_female parameters to checkParentIDs, allowing users to specify custom values for male and female sex coding. Updated documentation and NEWS to reflect this change. Improved handling of missing sex values in checkParentSex.
7cc9405 to
d212334
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
- Coverage 84.45% 84.42% -0.03%
==========================================
Files 25 25
Lines 3957 3982 +25
==========================================
+ Hits 3342 3362 +20
- Misses 615 620 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Introduced the 'code_unknown' parameter to checkSex, repairSex, and recodeSex functions, allowing explicit handling and recoding of unknown sex values. Updated documentation to reflect the new parameter and its usage. Should help address R-Computing-Lab/ggpedigree#95
c248db1 to
4060a62
Compare
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 adds customizable sex coding parameters to the checkParentIDs function and related sex-handling functions in the BGmisc package. The changes introduce code_male and code_female parameters to checkParentIDs, and code_unknown parameter to multiple sex-related functions (checkSex, repairSex, recodeSex), allowing users to specify custom values for different sex categories rather than relying solely on inferred values from the pedigree data.
Key changes include:
- Added
code_maleandcode_femaleparameters tocheckParentIDsto allow explicit specification of sex codes - Introduced
code_unknownparameter tocheckSex,repairSex, andrecodeSexfunctions for handling unknown sex values - Enhanced
recodeSexfunction logic to handle unknown sex values with multiple code paths - Improved
checkParentSexto handle cases where all parents have missing sex values
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| R/checkParents.R | Added code_male and code_female parameters to checkParentIDs function; updated logic to use provided sex codes instead of only inferred values |
| R/checkSex.R | Added code_unknown parameter to checkSex, repairSex, and recodeSex functions; restructured recodeSex logic to handle unknown sex values; improved checkParentSex to handle missing values |
| man/checkParentIDs.Rd | Added documentation for new code_male and code_female parameters |
| man/checkSex.Rd | Added documentation for new code_unknown parameter; removed trailing whitespace |
| man/recodeSex.Rd | Added documentation for code_unknown and recode_unknown parameters; removed trailing whitespace |
| man/repairSex.Rd | Added code_unknown parameter to function signature and documentation; removed trailing whitespace |
| NEWS.md | Updated version entry to document sex coding enhancements |
man/checkParentIDs.Rd
Outdated
| \item{code_male}{Value representing male sex} | ||
|
|
||
| \item{code_female}{Value representing female sex} |
Copilot
AI
Jan 2, 2026
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 documentation for code_male and code_female parameters is incomplete. They should specify what they represent (e.g., "The code value used to represent male sex in the 'sex' column" and "The code value used to represent female sex in the 'sex' column").
| \item{code_male}{Value representing male sex} | |
| \item{code_female}{Value representing female sex} | |
| \item{code_male}{The code value used to represent male sex in the \code{sex} column of \code{ped}.} | |
| \item{code_female}{The code value used to represent female sex in the \code{sex} column of \code{ped}.} |
| dadID = "dadID", | ||
| code_male = NULL, | ||
| code_female = NULL | ||
| ) { |
Copilot
AI
Jan 2, 2026
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 indentation is inconsistent with the closing parenthesis. It should align with the function name or be properly indented.
| ) { | |
| ) { |
| code_male = validation_results$most_frequent_sex_dad, | ||
| code_female = validation_results$most_frequent_sex_mom, | ||
| code_unknown = code_unknown | ||
| ) |
Copilot
AI
Jan 2, 2026
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 indentation for closing parenthesis should be consistent with the opening line or properly aligned. This closing parenthesis should align with the opening function call on line 98.
| ped$sex_recode[!ped$sex %in% c(code_male, code_female) & !is.na(ped$sex)] <- recode_unknown | ||
| } | ||
|
|
||
|
|
Copilot
AI
Jan 2, 2026
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.
Empty line here serves no purpose and reduces code readability. Consider removing this blank line.
| code_male = NULL, | ||
| code_female = NULL | ||
| ) { |
Copilot
AI
Jan 2, 2026
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 new code_male and code_female parameters in checkParentIDs lack test coverage. Tests should verify that when these parameters are provided, they are properly used to set validation_results$male_var and validation_results$female_var instead of inferring them from the data.
|
|
||
|
|
||
|
|
Copilot
AI
Jan 2, 2026
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.
Empty lines here serve no purpose and reduce code readability. Consider removing these blank lines.
| recodeSex <- function( | ||
| ped, verbose = FALSE, code_male = NULL, code_na = NULL, code_female = NULL, | ||
| recode_male = "M", recode_female = "F", recode_na = NA_character_) { | ||
| ped, verbose = FALSE, code_male = NULL, code_na = NULL, code_female = NULL, | ||
| code_unknown = NULL, | ||
| recode_male = "M", | ||
| recode_female = "F", | ||
| recode_unknown = "U", | ||
| recode_na = NA_character_ | ||
| ) { | ||
| if (is.null(code_male) && is.null(code_female)) { | ||
| if (verbose == TRUE) { | ||
| warning("Both code male and code female are empty. No recoding was done.") | ||
| } | ||
| return(ped) | ||
| } | ||
| # First, set any code_na values to NA | ||
| if (!is.null(code_na)) { | ||
| ped$sex[ped$sex == code_na] <- NA | ||
| } | ||
| # Recode as "F" or "M" based on code_male, preserving NAs | ||
| if (!is.null(code_male) && !is.null(code_female)) { | ||
| # Initialize sex_recode as NA, preserving the length of the 'sex' column | ||
| ped$sex_recode <- recode_na | ||
| ped$sex_recode[ped$sex == code_female] <- recode_female | ||
| ped$sex_recode[ped$sex == code_male] <- recode_male | ||
| # Overwriting temp recode variable | ||
| ped$sex <- ped$sex_recode | ||
| ped$sex_recode <- NULL | ||
| } else if (!is.null(code_male) && is.null(code_female)) { | ||
| # Initialize sex_recode as NA, preserving the length of the 'sex' column | ||
| ped$sex_recode <- recode_na | ||
| ped$sex_recode[ped$sex != code_male & !is.na(ped$sex)] <- recode_female | ||
|
|
||
| # Initialize sex_recode as NA, preserving the length of the 'sex' column | ||
| ped$sex_recode <- recode_na | ||
|
|
||
|
|
||
| if (!is.null(code_male)) { | ||
| ped$sex_recode[ped$sex == code_male] <- recode_male | ||
| # Overwriting temp recode variable | ||
| ped$sex <- ped$sex_recode | ||
| ped$sex_recode <- NULL | ||
| } else if (is.null(code_male) && !is.null(code_female)) { | ||
| # Initialize sex_recode as NA, preserving the length of the 'sex' column | ||
| ped$sex_recode <- recode_na | ||
| ped$sex_recode[ped$sex != code_female & !is.na(ped$sex)] <- recode_male | ||
| } | ||
| if (!is.null(code_female)) { | ||
| ped$sex_recode[ped$sex == code_female] <- recode_female | ||
| # Overwriting temp recode variable | ||
| ped$sex <- ped$sex_recode | ||
| ped$sex_recode <- NULL | ||
| } else { | ||
| if (verbose == TRUE) { | ||
| warning("Both code male and code female are empty. No recoding was done.") | ||
| } | ||
|
|
||
| # handle unknown codes | ||
| if (!is.null(code_unknown) && !is.na(code_unknown)) { | ||
| ped$sex_recode[ped$sex == code_unknown] <- recode_unknown | ||
| } else if (!is.null(code_unknown) && is.na(code_unknown)) { | ||
| ped$sex_recode[is.na(ped$sex)] <- recode_unknown | ||
| } else if (!is.null(code_male) && !is.null(code_female)) { | ||
| ped$sex_recode[!ped$sex %in% c(code_male, code_female) & !is.na(ped$sex)] <- recode_unknown | ||
| } | ||
|
|
||
|
|
||
| # Handle cases where only one of code | ||
| # just male | ||
| if (!is.null(code_male) && is.null(code_female)) { | ||
| if (!is.null(code_unknown)) { | ||
| ped$sex_recode[ped$sex != code_male & !is.na(ped$sex) & ped$sex != code_unknown] <- recode_female | ||
| } else if (is.null(code_unknown)) { | ||
| ped$sex_recode[ped$sex != code_male & !is.na(ped$sex)] <- recode_female | ||
| } | ||
| } | ||
| # just female | ||
| if (is.null(code_male) && !is.null(code_female)) { | ||
| if (!is.null(code_unknown)) { | ||
| ped$sex_recode[ped$sex != code_female & !is.na(ped$sex) & ped$sex != code_unknown] <- recode_male | ||
| } else if (is.null(code_unknown)) { | ||
| ped$sex_recode[ped$sex != code_female & !is.na(ped$sex)] <- recode_male | ||
| } | ||
| } |
Copilot
AI
Jan 2, 2026
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 new code_unknown parameter and recode_unknown parameter lack test coverage. Tests should verify the behavior when code_unknown is provided, including the edge cases where code_unknown is NA (lines 198-199) and when it needs to infer unknown values from the data (lines 200-201).
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.
@copilot open a new pull request to apply changes based on this feedback
|
@smasongarrison I've opened a new pull request, #107, to work on those changes. Once the pull request is ready, I'll request review from you. |
adacf1d to
79ab419
Compare
Update R/checkParents.R Update R/checkParents.R Co-Authored-By: Copilot <175728472+Copilot@users.noreply.github.com>
79ab419 to
e37cd10
Compare
…ecodeSex (#107) * Update R/checkParents.R Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update R/checkParents.R Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Initial plan * Add comprehensive test coverage for code_unknown and recode_unknown parameters Co-authored-by: smasongarrison <6001608+smasongarrison@users.noreply.github.com> * Address code review feedback: improve documentation and test comments Co-authored-by: smasongarrison <6001608+smasongarrison@users.noreply.github.com> * Remove brittle line number references from test comments Co-authored-by: smasongarrison <6001608+smasongarrison@users.noreply.github.com> --------- Co-authored-by: Mason Garrison <garrissm@wfu.edu> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: smasongarrison <6001608+smasongarrison@users.noreply.github.com>
Introduced code_male and code_female parameters to checkParentIDs, allowing users to specify custom values for male and female sex coding. Updated documentation and NEWS to reflect this change. Improved handling of missing sex values in checkParentSex.