Skip to content

Fix aggregate_per_date() combining results using last file's columns … - #75

Open
fredho-42 wants to merge 1 commit into
PhysicalActivityOpenTools:mainfrom
fredho-42:fix/aggregate-per-date-column-order
Open

Fix aggregate_per_date() combining results using last file's columns …#75
fredho-42 wants to merge 1 commit into
PhysicalActivityOpenTools:mainfrom
fredho-42:fix/aggregate-per-date-column-order

Conversation

@fredho-42

Copy link
Copy Markdown

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).

…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 jhmigueles left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Successfully merging this pull request may close these issues.

2 participants