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

Fix midpoint calculation for cuts #19

Merged
merged 7 commits into from
Feb 24, 2022
Merged

Fix midpoint calculation for cuts #19

merged 7 commits into from
Feb 24, 2022

Conversation

arisp99
Copy link
Member

@arisp99 arisp99 commented Feb 17, 2022

This PR implements two large changes to our midpoint calculation of cuts. In using Hmisc::cut2() there are some occasions where a single cut is defined by the same lower and upper bound, i.e., the bucket is represented by one value. In an effort to combat this behavior, we previously found all buckets with one value and assigned them to another bucket. This PR reverts that behavior and treats each bucket as its own, no longer combining buckets together. Furthermore, we simplify the calculation to find the midpoints of our cuts. Since Hmisc::cut2() returns each cut as a factor, we use string processing to extract the lower and upper bounds and then compute the midpoint of the cut. In some cases, the cuts are not formatted uniformly so we take care to coerce the cuts to the same structure before determining the midpoints. The midpoint for a cut representing a single value is the value itself.

Closes #18.

In some cases, `Hmisc::cut2` assigns a cut where the lower and upper bound are the same. Previously we would combine cuts with one value into another cut. This is not needed.
This function will work even if cuts have only one value
It can be either a square or curved bracket.
The cuts output is needed for the ideal compute COI method
@arisp99 arisp99 requested a review from OJWatson February 17, 2022 18:52
Copy link
Collaborator

@OJWatson OJWatson left a comment

Choose a reason for hiding this comment

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

LGTM - also how nice are snapshots. Thanks for making me look that testing idea up

@arisp99 arisp99 merged commit e359dc2 into main Feb 24, 2022
@arisp99 arisp99 deleted the fix-midpoints branch February 24, 2022 15:53
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.

cuts issues in process
2 participants