search

Aarhus-Psychiatry-Research / timeseriesflattener

Converting irregularly spaced time series, such as eletronic health records, into dataframes for tabular classification.
https://Aarhus-Psychiatry-Research.github.io/timeseriesflattener
MIT License
18 stars 2 forks source link

Understanding of bug #559. #561

Closed MartinBernstorff closed 3 months ago

MartinBernstorff commented 3 months ago

Completely understand the merge here! #560

@HLasse, just adding some discussion here. Not sure I understand the fix.

Don't the temporal and static paths of the flattening get the prediction times in the same order, regardless of whether they are sorted or not?

HLasse commented 3 months ago

Yes and no. Only the temporal specs use the step_size argument; static does not. This means that the temporal specs will output the rows in the order of the prediction times, but sorted by the steps (e.g. if step_size=1 year, 2013 first, 2014, ...), whereas static specs always output in the same order as the prediction times. If prediction times are not temporally ordered there will therefore be a mismatch between the output of temporal and static specs.

MartinBernstorff commented 3 months ago

Ohhh, I see! Of course, this results in implicit sorting. Great catch, and thanks so much for fixing it. Apologies for introducing this bug, neither our unit tests, nor @sarakolding or I caught it.

As a learning experience, this indicates the need for interaction-testing. Really happy you added that here.

HLasse commented 3 months ago

Yeah, a sneaky one!! Goes to show the value of collaborative work ❤️