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

option to identify all possible overlapping intervals #70

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

chacalle
Copy link
Collaborator

@chacalle chacalle commented Sep 17, 2021

Describe changes

To help with overlapping intervals where it may not be clear what is truly the less granular interval.

Like: 0-11, 10-15, 15-20 as specified in #66.

We can visually inspect and know 0-11 is the messed up age group likely, but before this PR only 10-15 was flagged as overlapping. Now if identify_all_possible=TRUE is specified, both 0-11 and 10-15 are flagged for manual inspection.

Checklist

Packages Repositories

  • Have you read the contributing guidelines for ihmeuw-demographics R packages?
  • Have you successfully run devtools::check() locally?
  • Have you updated or added function (and vignette if applicable) documentation? Did you update the 'man' and 'NAMESPACE' files with devtools::document()?
  • Have you added in tests for the changes included in the PR?
  • Do the changes follow the ihmeuw-demographics code style?
  • Do the changes need to be immediately included in a new build of docker-base or docker-internal? If so follow directions in those repositories to rebuild and redeploy the images.
  • Do the changes require updates to other repositories which use this package? If yes, make the necessary updates in those repos, and consider integration tests for those repositories.
  • If this is a private package did you use Jenkins to rebuild the internal pkgdown site?

@chacalle chacalle added the enhancement New feature or request label Sep 17, 2021
@chacalle chacalle self-assigned this Sep 17, 2021
@@ -137,7 +137,7 @@ collapse_common_intervals <- function(dt,
# check for overlapping intervals
if (overlapping_dt_severity != "skip") {
overlapping_dt <- dt[
, identify_overlapping_intervals(unique(.SD)),
, identify_overlapping_intervals(unique(.SD), identify_all_possible = overlapping_dt_severity != "none"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message on 151 is only triggered if overlapping_dt_severity != "none". We want the error message to show all possible overlapping intervals.

But if the error is skipped then line 155 tries to drop the least granular overlapping interval.

@chacalle chacalle changed the base branch from master to main September 17, 2021 21:22
@chacalle chacalle merged commit 79f88bf into main Sep 20, 2021
@chacalle chacalle deleted the feature/all_overlapping_intervals branch September 20, 2021 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants