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

Don't warn about coercing j to integer #6594

Open
MichaelChirico opened this issue Oct 31, 2024 · 0 comments · May be fixed by #6595
Open

Don't warn about coercing j to integer #6594

MichaelChirico opened this issue Oct 31, 2024 · 0 comments · May be fixed by #6595

Comments

@MichaelChirico
Copy link
Member

MichaelChirico commented Oct 31, 2024

warning(_("Coerced j from numeric to integer. Please pass integer for efficiency; e.g., 2L rather than 2"));

I find this warning a bit annoying. It's very common in R to have double "just work" as integer. Why this of all places is there an efficiency concern?

I can't imagine the efficiency concern is real anyway -- unless we are coercing 1e7+ column numbers to integer I can't imagine there's a real performance implication.

I also see two cases where this might come up:

  1. You are passing j as a variable to set():

    setDT(DT, , col_idx, val)

    In this case, the recommendation is to do as.integer(col_idx), which is what assign.c is doing implicitly. Is there really any difference in efficiency?

  2. You are doing interactive work (as I just was when I came across this) and lazily omitting the L:

    setDT(DT, , c(1, 4), NULL)

    OK, I could write 1L/4L, and in package/production code, I probably should. But to throw a warning seems a bit heavy-handed. Users really under such use cases should be using lintr::implicit_integer_linter() to catch such cases.

Originally added a long time ago: e2cf7e2

@MichaelChirico MichaelChirico linked a pull request Oct 31, 2024 that will close this issue
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 a pull request may close this issue.

1 participant