wadpac / GGIR

Code corresponding to R package GGIR
https://wadpac.github.io/GGIR/
Apache License 2.0
94 stars 60 forks source link

TimeSegments2ZeroFile parameter #1069

Closed jhmigueles closed 5 months ago

jhmigueles commented 6 months ago

TimeSegments2ZeroFile actually expects a data.frame, and not a file path, thus the name of the parameter is confusing.

vincentvanhees commented 6 months ago

I think option 2 is preferable, because that avoids having to change parameter names and broadens the functionality. I am sure people will like the option to provide files instead of data.frames but those who have used data.frame will like to keep doing so.

I assume this is not going to be an enormous time investment?

jhmigueles commented 5 months ago

check_params, in line 115 checks that TimeSegments2ZeroFile is a character argument and triggers an error in case it is not. This means that users were not able to provide data frames to TimeSegments2ZeroFile in the past, right? Should I skip the check on the class of TimeSegments2ZeroFile to allow for both data frames and file paths? Or should I keep the error and introduce new parameter TimeSegments2ZeroDataFrame (informing that they should be using this parameter instead)?

vincentvanhees commented 5 months ago

If nobody has been able to use it and nobody ever complained about it then maybe best is to remove the functionality to help tidy up GGIR. The less functionality we have to maintain the better.

jhmigueles commented 5 months ago

Can we be sure about that? The function might have been used before implementing this check?

vincentvanhees commented 5 months ago

Ok, I just looked inside the code and see that in g.part2 parameter TimeSegments2ZeroFile is actually expected to be a file, so the problem is with the parameter documentation and not with the functionality itself. It is not a data.frame that should be a supplied but the path to a csv file with the following columns:

The assumption seems to be that doing as.POSIXlt(windowstart, tz = desiredtz) automatically works without the need to specify a timestamp format. So, presumably the assumption is that it follows the machines default timestamp format?

jhmigueles commented 5 months ago

I can try with files using time stamps in different formats and see if everything works. If so, then we only need to revise documentation, right?

jhmigueles commented 5 months ago

Only testing for now, I created the TimeSegments2ZeroFile using different classes (as.POSIXct, as.POSIXlt, and simple character strings with the timestamps) but the same format ("%Y-%m-%d %H:%M:%S"). Here an example:

# as.POSIXct
ts2zero = data.frame(filename = "01gt3x.gt3x.RData",
                     windowstart = as.POSIXct("2023-02-22 18:00:00", tz = "Europe/Madrid"),
                     windowend= as.POSIXct("2023-02-22 22:00:00", tz = "Europe/Madrid"))
write.csv(ts2zero, "D:/GGIRdev/logs/ts2zero.csv", row.names = FALSE)

All these formats could be read, I guess because the format of the time stamps is the same. However, if I change the format to, for example "%d/%m/%Y %H:%M:%S", then the time stamps are not correctly identified and the TimeSegments2ZeroFile is not used.

Other observations:

vincentvanhees commented 5 months ago

I looked into my email correspondence and see that the functional as implemented is in line with the requirements of a project I did in 2019. The main problems are: