-
Notifications
You must be signed in to change notification settings - Fork 779
AnomalyDetectionTs drops timezone from POSIXct objects and converts POSIXlt to POSIXct #2
Comments
Thanks, Owen and I will look into it Zach! |
Thanks! |
This looks like a useful package, thanks! Perhaps the 'timestamp' column should be required to be of class POSIXct? I don't believe base::merge works when the 'by' column is of class POSIXlt. For example:
With the timezone issue, if the 'timestamp' column is POSIXct, the timezone on the object returned can be made to match that of the input data. If the timestamp column is not POSIXct, it could be converted using as.POSIXct instead of strptime in format_timestamp, and given a default time zone of UTC possibly. |
@erikeldridge There's really 2 issues here:
|
@erikeldridge I didn't see the last paragraph of your comment. I agree, if the timestamp is of class POSIXct, the timezone should be kept, and if the timestamp must be converted the timezone should be preserved if possible. |
Thanks for the comments guys -- Owen and I will look into this later today, but on glance you're right, we should preserve the timestamp/timezone. Stay tuned... |
@jhochenbaum Awesome, thank you! |
@zachmayer, would you like to submit a patch and @owenvallis and I can review? |
@jhochenbaum I haven't had time to take a crack on this yet, but if I do I'll submit a PR. |
Thanks @zachmayer |
@zachmayer After my PR #92, the
You should check timezone attribute for However, your code snippet exposed another problem of AnomalyDetection package, which is the input timestamp with non UTC timezone. The problem is explained by following code output.
As you see, the 1st row of I will try to fix the problem. |
I have fixed it by PR #92 Now the AnomalyDetectionTs keeps the original timezone attr as the input timestamp, see following output.
|
Hi team, Sorry for this up, I mean really. But with this script : library(AnomalyDetection) Here what I have : library(AnomalyDetection)
I cannot understand why, I've checked all what you've said, but no result... May I have some help ? |
@Maryoda2 The
Then rerunning your script will produce the expected results without errors (I tested your script). |
Oh thanks a lot ! |
Well... in fact sometimes it writes : Error in aggregate.data.frame(x[2], format(x[1], "%Y-%m-%d %H:%M:00"), : |
@Maryoda2 could you give an example reproducing the error? |
library(AnomalyDetection) With another dataset, I've tried to translate your script. |
Please run |
|
This is because your timestamp is not in the format of "%Y-%m-%d %H:%M:00", which means |
Oh I see, How can I edit the code of twitter/AnomalyDetection, in fact the caijun/Twitter (yours) ? |
@Maryoda2 Could you share a sample of your data, which I can use for reproducing the error and testing? I think the problem you encountered has been reported in issue #77, which has been resolved by PRs #98 and #79 Although these PRs have not been merged into the twitter/AnomalyDetection master branch, but has been merged into isalgueiro/AnomalyDetection. You can try this fork of AnomalyDetection by
|
Well thanks for your answer |
@Maryoda2 the data you provided is not enough to reproduce the error. Please provide data with at least 20000 rows. |
20.000 is no more a sample, but we can't detect anomalies on dataset less than 20.000 ? |
@Maryoda2 Using the data you provided, I encountered the error "Anom detection needs at least 2 periods worth of data". Hence, I need more data to make sure this error was not because of limited data. |
In fact for the moment this is all what I have... So game over then ? |
replace calls to 'subset()' with '['
AnomalyDetectionTs should keep the timestamp column of the output dataset as-is, rather than converting to POSIXct and dropping the timezone attribute:
Dropping the timezone is problematic if you wish to merge the anomalies back to the main dataset:
The text was updated successfully, but these errors were encountered: