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

flast returns wrong values for POSIXct columns #91

Closed
harvey131 opened this issue Nov 13, 2020 · 6 comments
Closed

flast returns wrong values for POSIXct columns #91

harvey131 opened this issue Nov 13, 2020 · 6 comments

Comments

@harvey131
Copy link

harvey131 commented Nov 13, 2020

The last value returned by flast for posixct columns in this case the date column) is not the last value for the group.

x <- mtcars 
x$date = Sys.time() + 1:nrow(x) 
tail(x,1) 
collapse::collap(x, ~cyl, FUN = collapse::flast) 

# this will return the correct value:
 collapse::flast(x, x[,'cyl'])

I am using version 1.2.1

@harvey131 harvey131 changed the title flast returns wrong valeus with POSIXIct columns flast returns wrong values for POSIXct columns Nov 13, 2020
@SebKrantz
Copy link
Owner

Hi, thanks for flagging this. So first of all I hope I made it clear that you should not be using collap() like this. Please comprehend the answer I have given you on your previous question if this remains unclear. Then you have indeed flagged an interesting issue. It seems that repeated execution of collapse::flast(x, x[,'cyl']) even gives slightly different second counts. So there seems to be an issue of numerical precision here since dates are stored as doubles in C. This is even stranger as flast() actually does not even perform a computation but simple copies a value to the result vector. I will investigate this and try to fix it in the next release (which will take more than a month since collapse 1.4 has just been released).

@SebKrantz
Copy link
Owner

Hello, can you install collapse 1.5.0 and check this again for me? It seems to me the issue has vanished.

@SebKrantz
Copy link
Owner

Any update from your side? Otherwise I'll close this issue as it seems to have been resolved.

@harvey131
Copy link
Author

harvey131 commented Jan 13, 2021

It still looks like an issue to me.

> x <- mtcars 
> x$date = Sys.time() + 1:nrow(x) 
> tail(x,1) 
            mpg cyl disp  hp drat   wt qsec vs am gear carb                date
Volvo 142E 21.4   4  121 109 4.11 2.78 18.6  1  1    4    2 2021-01-13 11:15:19
> collapse::collap(x, ~cyl, FUN = collapse::flast) 
   mpg cyl disp  hp drat   wt qsec vs am gear carb                date
1 21.4   4  121 109 4.11 2.78 18.6  1  1    4    2 2021-01-13 11:14:50
2 19.7   6  145 175 3.62 2.77 15.5  0  1    5    6 2021-01-13 11:14:48
3 15.0   8  301 335 3.54 3.57 14.6  0  1    5    8 2021-01-13 11:14:52
> collapse::collap(x, ~cyl, FUN = 'flast')
   mpg cyl disp  hp drat   wt qsec vs am gear carb                date
1 21.4   4  121 109 4.11 2.78 18.6  1  1    4    2 2021-01-13 11:14:50
2 19.7   6  145 175 3.62 2.77 15.5  0  1    5    6 2021-01-13 11:14:48
3 15.0   8  301 335 3.54 3.57 14.6  0  1    5    8 2021-01-13 11:14:52
> # this will return the correct value:
> collapse::flast(x, x[,'cyl'])
   mpg cyl disp  hp drat   wt qsec vs am gear carb                date
4 21.4   4  121 109 4.11 2.78 18.6  1  1    4    2 2021-01-13 11:15:19
6 19.7   6  145 175 3.62 2.77 15.5  0  1    5    6 2021-01-13 11:15:17
8 15.0   8  301 335 3.54 3.57 14.6  0  1    5    8 2021-01-13 11:15:18
> packageVersion('collapse')
[1] ‘1.5.0’

@SebKrantz
Copy link
Owner

Ok, thanks. It seems actually all statistical functions give the same set of dates. So it is definitely not an error in the code, but it appears to be that POSIXct classes require special treatment so just selecting the last double value and copying the attributes does not give you the right computation. I can try to add a special case for POSIXct, but it may take a while for me to understand the mechanics of it. So for the moment collapse can unfortunately not deal properly with POSIXct.

@SebKrantz
Copy link
Owner

Hello, so I have found the issue: I used a C API Macro SHALLOW_DUPLICATE_ATTRIB() to copy object attributes, which is more efficient but does not copy the object bits needed for classed objects. I now replaced this with DUPLICATE_ATTRIB() which does a better job. The only functions where I kept the old macro were fvar and fsd where I think applying them to dates does not make sense. Otherwise all other functions now aupport POSIXct and other classes. This is available now in the development version on github, and will go on CRAN in an update by mid next month.

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

No branches or pull requests

2 participants