Add tests of forder NA/NaN behavior

[MichaelChirico]

Also, all the TODOs here are now pulled into #2572, so removing them from the tests file (where they're less visible)

[codecov]

Codecov Report

Merging #3409 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3409   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files          65       65           
  Lines       12127    12127           
=======================================
  Hits        11536    11536           
  Misses        591      591

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4b195f...4fafb87. Read the comment docs.

[codecov]

Codecov Report

Merging #3409 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3409   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files          65       65           
  Lines       12127    12127           
=======================================
  Hits        11536    11536           
  Misses        591      591

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4b195f...4fafb87. Read the comment docs.

[jangorecki]

those new tests, they weren't just dput(forder()) right?

[MichaelChirico]

they weren't just dput(forder()) right?

They were 😬

[jangorecki]

So just please examine if returned values are expected ones. Such methodology of adding tests does tests only that returned values will not change over time but not if they are correct. It happens few times to me that I pushed incorrect tests like that.

[MichaelChirico]

Thanks @jangorecki. I actually think current behavior is inconsistent:

x = data.table(r = c(6, 4, 2, NA, 1, NaN, 5, NaN, 9, 10, NA))

x[order(r, na.last = TRUE)]
#         r
#     <num>
#  1:     1
#  2:     2
#  3:     4
#  4:     5
#  5:     6
#  6:     9
#  7:    10
#  8:   NaN
#  9:   NaN
# 10:    NA
# 11:    NA
x[order(r, na.last = FALSE)]
#         r
#     <num>
#  1:    NA
#  2:    NA
#  3:   NaN
#  4:   NaN
#  5:     1
#  6:     2
#  7:     4
#  8:     5
#  9:     6
# 10:     9
# 11:    10
x[order(-r, na.last = TRUE)]
#         r
#     <num>
#  1:    10
#  2:     9
#  3:     6
#  4:     5
#  5:     4
#  6:     2
#  7:     1
#  8:   NaN
#  9:   NaN
# 10:    NA
# 11:    NA
x[order(-r, na.last = FALSE)]
#         r
#     <num>
#  1:    NA
#  2:    NA
#  3:   NaN
#  4:   NaN
#  5:    10
#  6:     9
#  7:     6
#  8:     5
#  9:     4
# 10:     2
# 11:     1

If NA > NaN, 3rd (x[order(-r, na.last = TRUE)]) looks wrong. Similarly for the 2nd one looking off.

[jangorecki]

I renamed to WIP so it won't be yet merged, lets first agree on expected output.

[mattdowle]

Great discussion. I think of the ordering of NA and NaN as different to the ordering of the finite numbers: if na.last is provided then NA and NaN are put at the beginning or the end in a fixed order such that NA is always at the end with NaN before the NA. I doubt it matters really, just that the choice is consistent.
I installed both v1.9.8 and v1.11.8 and checked the results checked in this PR match the results then. All good. So for the reason of consistency over time, these tests look good to me.