Issue745 epoch part5 #901
Issue745 epoch part5 #901
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #901 +/- ##
==========================================
- Coverage 78.22% 78.21% -0.01%
==========================================
Files 92 94 +2
Lines 12301 12337 +36
==========================================
+ Hits 9623 9650 +27
- Misses 2678 2687 +9
|
… in part 5 reports
There was a problem hiding this comment.
Thanks for having a go at this @jhmigueles. It may be easier to discuss first offline.
My concern is that the added loops and if statements reduce readability and increase complexity of the code a lot. Although this may work for what we currently want, it would make it really hard to make modifications to allow for new variations on this without adding ever more if statements and loops.
I am wondering whether it is worth all the effort, because we eventually have to deal with all this complexity when we fix bugs and implement other enhancements. The user has the option to re-run part 5, so it is not like GGIR does not facilitate the epochsize.
I may need to think more about this, but I am wondering whether there would be a way to create all resolutions of ts in a single place and to put them in a list ts_list, which is then forwarded to all existing functions. There we then have full freedom to use whichever resolution we may want without being constrained to where we are in the code.
Advantages:
- No extra loops or if-statements in g.part 5
- Full flexibility across part 5 to take advantage of multiple resolution data. For example, we may want to do fragmentation analysis during the day at 60 second resolution, and during the night at 30 second resolution, while time-use analysis at 5 second resolution. Similarly, we may at some point want to combine 5 second and 60 resolution data for time-use analysis.
Disadvantage:
- Even the approach I describe here still adds complexity as we need to consider carefully what epoch size is used for each part of the code.
- Exporting time series currently assumes 1 epoch size, but in the new situation we need to ask user which epoch size the exported time series should have.
EDIT: Another route may be:
- Add option inside g.fragmentation to only aggregate to 60 seconds there. In that way user can use 5 seconds for time-use and 60 seconds for fragmentation with only one new argument, and all code enhancements specific to g.fragmentation.
| aggregate_to_epoch = function(ts, to, lightpeak_available, params_general) { | ||
|
|
||
| # Aggregate time | ||
| ts$time_num = floor(as.numeric(iso8601chartime2POSIX(ts$time,tz = params_general[["desiredtz"]])) / to) * to |
There was a problem hiding this comment.
Timestamp format conversion should not be here. The name of the function only refers to aggregation to epoch and we risk doing the same conversion multiple times. I think this should be done centrally inside for example g.part5_initialise_ts.
| @@ -0,0 +1,21 @@ | |||
| extract_nightsi = function(ts, dayborder, desiredtz, tail_expansion_log) { | |||
There was a problem hiding this comment.
Why not use or enhance g.detecmidnight? This seems to overlap.
| if (ws3new == params_general[["part5_epochSizes"]][1]) classifier = c(classifier, "timeuse") | ||
| if (ws3new == params_general[["part5_epochSizes"]][2]) classifier = c(classifier, "fragmentation") | ||
| if (ws3new != ws3) { | ||
| ts = aggregate_to_epoch(ts, ws3new, lightpeak_available, params_general) |
There was a problem hiding this comment.
You re-use ts from the previous iteration of the loop, which could cause problems:
- If epochi is not ranked from low to high
- First aggregating to, let's say 30 and then to 60 may not be the same as aggregating from 5 to 60 directly.
So, if you do this then I would go back to original ts and aggregate from there. - If you want to export ts at high resolution.
Closes #745 => Enhanced functionality to aggregate epoch size in part 5. The new argument
part5_epochSizesrequire 2 numeric values indicating the epoch length for timeuse and for fragmentation analysis in part 5.part5_agg2_60secondsis set toTRUE, then GGIR automatically setspart5_epochSizes = c(60, 60)and returns a warning.part5_epochSizesdefinition is not a multiple of thewindowsizes[1], e.g.,windowsizes[1] = 5andpart5_epochSizes = c(8, 60), then aggregation is not possible. In this case, GGIR setspart5_epochSizes = c(windowsizes[1], 60)and returns a warning. In the unlikely event that 60 is not a multiple ofwindowsizes[1]either, thenpart5_epochSizes = c(windowsizes[1], windowsizes[1])Checklist before merging:
inst/NEWS.Rdwith a user-readable summary. Please, include references to relevant issues or PR discussions.DESCRIPTIONandCITATION.cfffiles.