merge.xts does not error if NA occurs at the end of index #174
Comments
joshuaulrich
added a commit
that referenced
this issue
Mar 31, 2017
There were cases where merge.xts would not throw an error if the index contained NA values. One case was if the index was integer, because the section of code that checked for integer NA was referencing the real_*index references instead of int_*index. After fixing that bug, the issue still remained for integer indexes. Now the issue is that NA_INTEGER = INT_MIN, so logical operations still work. But they don't give the results we want. Fix the issue by checking for NA_INTEGER before any other logical comparisons. Added similar checks for the real/double index case as well, and also moved the check to the top of the if/else branch. Note that the test now uses R_FINITE to check for NaN and +/- as well. I really do not like these checks in the first position of the if/else branch because they should never happen in user space. For performance, the most commonly true if statements should come first. Need to look for an upstream place to check for NA in the index. Fixes #174.
joshuaulrich
added a commit
that referenced
this issue
May 16, 2017
There were cases where merge.xts would not throw an error if the index contained NA values. One case was if the index was integer, because the section of code that checked for integer NA was referencing the real_*index references instead of int_*index. After fixing that bug, the issue still remained for integer indexes. Now the issue is that NA_INTEGER = INT_MIN, so logical operations still work. But they don't give the results we want. Fix the issue by checking for NA_INTEGER before any other logical comparisons. Added similar checks for the real/double index case as well, and also moved the check to the top of the if/else branch. Note that the test now uses R_FINITE to check for NaN and +/-Inf as well. I really do not like these checks in the first position of the if/else branch because they should never happen in user space. For performance, the most commonly true if statements should come first. Need to look for an upstream place to check for NA in the index. Fixes #174.
joshuaulrich
added a commit
that referenced
this issue
Jun 7, 2017
Checking for NA, NaN, and +/-Inf values in the first position of the if-else statement was a bad idea for performance reasons, but was also incorrect. Any time two merged objects had different start or end index values, xp or yp ar outside the bounds of their respective index vectors. That didn't cause a segfault though, because R_FINITE() would happily interpret the random memory as either a number or NaN. This would manifest as an error when there should be none, or an infinite loop. We can fix the logic and improve performance by taking advantage of the fact that the indexes are sorted. Therefore, if present, -Inf will always be in the first element, while NA, Inf, and NaN will always be in the last element. In the case of an integer index, the NA will always be last (because that's how it sorts in R, even though it's equal to INT_MIN in C). See #174.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
merge.xts()should throw an error if there are anyNAin the index, and it does most of the time. Strangely, it "works" if theNAis at the end of the index, and the xts object was created with the.xts()constructor.Fixing #173 will avoid this entirely, but it should still be investigated in case another bug is lurking in the darkness...
The text was updated successfully, but these errors were encountered: