Fix aggregate_per_date() combining results using last file's columns … - #75
Conversation
…instead of first After looping over all time-series files, aggregate_per_date() combined per-file results and set the final table's column count/names from whichever file happened to be processed *last* (ci/dsnames were reused directly from the final loop iteration). This made the result silently depend on file processing order/completeness. Now each file's (ds, dsnames) pair is collected in a list and combined afterwards with purrr::list_rbind(), using the *first* file's dsnames as the canonical column names/count. Verified against a synthetic multi-subject dataset: output is identical to the previous implementation in the well-behaved case (same column count for every file).
jhmigueles
left a comment
There was a problem hiding this comment.
Thanks for working on this. Before merging this PR I think we need to clarify the underlying problem and also expand the PR beyond the function-level change.
Could you first clarify the actual bug?
I am not fully understanding what is the issue with the dependency on the last file. In aggregate_per_date(), ds, ci, and dsnames are recalculated inside the loop for every file. However, the column structure is determined by the same function-level arguments (classifier, classes, and boutdur) for every iteration. Therefore, for a given call to aggregate_per_date(), I would expect every file to generate the same ci and dsnames, regardless of the order in which the files are processed.
Therefore, I don't see why using the first file's dsnames is safer than using the last file's dsnames. Could you please provide a minimal reproducible example showing a situation where two files processed by the same call generate different ci/dsnames, and how the current implementation produces an incorrect result?
Also, could you clarify what you mean in the PR description by
"and won't be compatible when there're multiple bout lengths"
boutdur can already be a vector of multiple bout lengths, and these are processed for every file in exactly the same way, producing the same columns across files.
Additional requirements for the PR
More generally, for changes like this I would prefer to first open an issue describing the bug, the current and expected behaviour, and a minimal reproducible example. This allows us to agree on the intended behaviour before implementing it.
The PR can then implement the agreed change and should include:
- the relevant tests
- documentation updates
- a NEWS entry
After looping over all time-series files, aggregate_per_date() combined per-file results and set the final table's column count/names from whichever file happened to be processed last (ci/dsnames were reused directly from the final loop iteration). This made the result silently depend on file processing order/completeness, and won't be compatible when there're multiple bout lengths.
Now each file's (ds, dsnames) pair is collected in a list and combined afterwards with purrr::list_rbind(), using the first file's dsnames as the canonical column names/count.
Verified against a synthetic multi-subject dataset: output is identical to the previous implementation in the well-behaved case (same column count for every file).