i.realDouble joins to x.int again by coercing x.int to double #3846
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3846 +/- ##
==========================================
+ Coverage 99.42% 99.42% +<.01%
==========================================
Files 71 71
Lines 13408 13409 +1
==========================================
+ Hits 13331 13332 +1
Misses 77 77
Continue to review full report at Codecov.
|
|
cc @HughParsonage as |
MichaelChirico
left a comment
MichaelChirico
left a comment
There was a problem hiding this comment.
Do we have the rules about type mismatch written into the documentation anywhere? Maybe a section in ?merge.data.table?
| set(i, j=ic, value=val) | ||
| set(callersi, j=ic, value=val) # change the shallow copy of i up in [.data.table to reflect in the result, too. | ||
| } else { | ||
| if (verbose) cat("Coercing integer column x.",names(x)[xc]," to type double to match type of i.",names(i)[ic]," which contains fractions.\n",sep="") |
There was a problem hiding this comment.
Does setNumericRounding come into play here? If so we should note such (in the verbose message and/or in dox)
There was a problem hiding this comment.
Yes it comes into play. But it does anyway for regular double to double type-matching joins too. setNumericRounding is really just to resolve numeric accuracy; more like very-slight-inaccuracy. I'm not sure the wording to use to best convey this, or for the manual page. But yes agree would be good to add. I just got swamped by another 11 revdep fails in the full-rerun so I'll try and get through those as priority. The full-rerun included this PR and all the fails were unrelated, so that's good.
If you have something in mind for the verbose message here and/or manual page, feel free to add in a follow-up PR, or raise an issue to follow-up so we don't forget. Agree.
MarkusBonsch
left a comment
MarkusBonsch
left a comment
There was a problem hiding this comment.
I completely agree with the new behaviour that you propose.
Honestly, I wonder, why I implemented it differently since the table that I referenced in my second post in #2592 suggests the behaviour that you now implemented.
3 participants
Resolves
grattanin #3581In v1.12.2 when joining i.realDouble to x.int, fractions in i.realDouble would be truncated and would succeed in joining to those integers in x. That was widely agreed as wrong behavior. Then in dev we changed that to a type error in just the case when fractions are detected to be present in the i column. This PR finally gets the balance right, I hope, by coercing x.int to double instead so the numbers with fractions will not return a match, and the type and contents of the i column is still preserved in the result. There is a verbose message consistent with the other similar coercions too.
In future we could introduce an option to turn all of the verbose coercion messages in bmerge (not just this one) into warnings or errors. That way users can turn on the strict type-match-required option for production (to avoid wasteful coercions) without getting in the way of revdeps or new users.
The "Coerced" to "Coercing" change in verbose messages is just to make the verbose messages consistent. Chose "Coercing" to convey that the message is printed before the action and isn't past tense yet. If any of the coercions fails with an error for any reason then the verbose message will have just printed in current tense.