fix integer bins closed left #40
Conversation
|
|
||
| test_that("cut_width splits the last inclusive bin for integers where bin width is 1", { | ||
| ints <- c(0,1,2,2,3,3,3,4,4,4,4) | ||
| expectedBins <- c("[0,1)","[1,2)","[2,3)","[2,3)","[3,4)","[3,4)","[3,4)","[4,5]","[4,5]","[4,5]","[4,5]") |
There was a problem hiding this comment.
Since they're integers, can we have the last bin simply say "[4, 5)"? It makes sense that the last bin end is 5, but we know we added "5" to make the bin.
There was a problem hiding this comment.
thats default behavior of cut, for good reasons, and im not inclined to digging into that.
There was a problem hiding this comment.
can you explain what the reasons are? It doesn't make sense to me.
There was a problem hiding this comment.
for every other case, except the one where we have integers or dates w one value per bin, then either the first or last bin needs to be inclusive on both sides. and in fairness, bins w one discrete value per bin, arent really bins at all. i think what cut does is perfectly reasonable, for the vast majority of cases, and trying to run our own impl of a base r fxn feels like a bad idea.
There was a problem hiding this comment.
id rather contribute to base r for this edge case, and get the fix a year from now when it finally works its way through, than run our own impl of cut for that label.
There was a problem hiding this comment.
i did not mean to suggest we make our own version of cut or anything, i just didn't understand why having bins like "[4, 5]" were a good idea
There was a problem hiding this comment.
I'm not necessarily arguing they're a good idea, so much as they're not worth the effort to change. They're not very likely to actually confuse anyone, may not even be actually seen by anyone, but do cost me time to change, don't have a super clean way to change and do risk introducing bugs elsewhere by changing. I'd rather just keep relying on cut as-is to do its thing.
extends #37 to work for bins closed left as well