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

last() no longer dispatches if data.table::last masks xts::last #1347

Closed
joshuaulrich opened this issue Sep 20, 2015 · 8 comments
Closed

last() no longer dispatches if data.table::last masks xts::last #1347

joshuaulrich opened this issue Sep 20, 2015 · 8 comments
Labels
Milestone

Comments

@joshuaulrich
Copy link

library(xts)
library(data.table)
last(.xts(1:3,1:3))
# [1] 3
# but should be (was in 1.9.4):
#                     [,1]
#1969-12-31 18:00:03    3

It looks like the change in e011351 is the cause, and it seems like it's just an error in the if statement (|| should be &&).

But changing the || to && will cause xts::last.default to be used any time xts is on the search path, and xts::last.default is slower than data.table::last for vectors, lists, data.frame, or data.table. I think a better solution would have been to replace the call to stop with a call to tail(x, ...). Thoughts?

@arunsrinivasan
Copy link
Member

Joshua, yes that seems like a better fix to me too.

@arunsrinivasan arunsrinivasan added this to the v1.9.8 milestone Sep 22, 2015
@joshuaulrich
Copy link
Author

Would you like me to submit a PR?

@arunsrinivasan
Copy link
Member

If you can, sure :-). Else I / @jangorecki (?) can take care of it.

@jangorecki
Copy link
Member

Maybe something like this at the beginning of data.table::last

if (inherits(x, "xts") && "package:xts" %in% search())
    xts::last(x, ...)

@arunsrinivasan
Copy link
Member

@jangorecki Joshua has already provided the fix (to use tail()).

joshuaulrich added a commit to joshuaulrich/data.table that referenced this issue Sep 23, 2015
data.table::last is intended to work with xts::last, which is a S3
generic. e011351 appears to have caused a regression that prevents
xts::last from dispatching.

Revert e011351, but keep the is.data.frame check on the happy path, so
it will be fast for data.table objects. Return xts::last if xts is on
the search path. Otherwise, return tail(x, ...) instead of throwing an
error (the previous behavior).
@arunsrinivasan
Copy link
Member

Fixed now @joshuaulrich, please verify.

@joshuaulrich
Copy link
Author

@arunsrinivasan, that's essentially what I had in my commits, so it looks good to me! You were able to get to the unit test before I was... I got "distracted" by my job. :) Thanks!

@arunsrinivasan
Copy link
Member

sorry, dint see your commits...

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants