-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
na.locf incorrect when first column is entirely NA #233
Comments
Thanks a ton for using and troubleshooting the development code! It is an enormously valuable and helpful contribution to the project. Are you willing/able to add tests and attempt to fix? |
You're welcome. Thanks for developing it in the first place! Locally I think I fixed it by just deleting the check I mentioned. I can try to add a test but I need to get to grips with RUnit... |
Right, simply deleting that check makes sense. I believe that is left-over from when the function only operated on univariate objects. I'm not sure what you're doing to 'get to grips with RUnit'... I would hope you could copy and modify one of the existing tests. In this case, you should be able to modify the test I put in the comment to the first issue you opened. test.nalocf_first_column_all_NA <- function() {
nacol <- 1L
for (m in MODES) {
xdat <- XDAT2
xdat[,nacol] <- xdat[,nacol] * NA
storage.mode(xdat) <- m
zdat <- as.zoo(xdat)
x <- na.locf(xdat)
z <- na.locf(zdat)
checkEquals(x, as.xts(z), check.attributes = TRUE)
}
} |
I'm struggling to work out how to run the tests easily. Any pointers gratefully received. I can do R CMD build then R CMD check on the tar. That seems to give a different result to evaluating bits of the file in the test directory. |
For what it's worth, I trust the results of I know RStudio / devtools / testthat have ways to run individual tests, but I have encountered too many instances of users being confused because an updated package didn't load correctly when the package was already loaded in the current session. One thing you could do is something like this: Rscript -e 'library(xts); library(RUnit); source("xts/inst/unitTests/runit.na.locf.R"); test.nalocf_last_column_all_NA()` Sorry I don't have a better answer... |
The firstNonNA check only checks the first column for a 2-dimensional SEXP. Therefore if the first column is NA, the function returns without examining any other columns. Fix by removing this check. Fixes joshuaulrich#233
Description
New multi column na.locf doesn't work correctly when the first column is entirely NA. In this case, no filling is performed at all.
Expected behavior
Subsequent columns should be filled.
Minimal, reproducible example
Session Info
I think the problem is here:
https://github.com/joshuaulrich/xts/blob/master/src/na.c#L161
This checks whether the first non na is the first value in the second column and if so, returns.
PS Looking forward to this making into the stable release - seems to speed up some of my use cases massively. Thanks!
The text was updated successfully, but these errors were encountered: