wfdb-python icon indicating copy to clipboard operation
wfdb-python copied to clipboard

Update `MultiHeaderMixin` header formatting to match `HeaderMixin` formatting

Open briangow opened this issue 6 months ago • 4 comments

In 63c987f33098aa0ccded214115d7922a7d0b9325 updates to HeaderMixin were made to properly format the header date and time fields. This fix did not make it into MultiHeaderMixin (for multi-segment records). Therefore, writing a date using the wrheader method under MultiHeaderMixin currently results in writing the base_date as YYYY-MM-DD which isn't compatible with the DD/MM/YYYY format requirement.

This PR updated MultiHeaderMixin by using the code from HeaderMixin to properly set formats.

briangow avatar Jun 23 '25 19:06 briangow

I've added the duplicated preprocessing for the header fields to a standalone function for sustainability.

briangow avatar Oct 13 '25 14:10 briangow

See https://github.com/MIT-LCP/wfdb-python/pull/548 regarding the new test error.

briangow avatar Oct 13 '25 16:10 briangow

Not tested but it makes sense to me. Thanks!

The logic for reformatting date strings looks fragile and I wonder if there's a reason it's not just using strftime("%d/%m/%Y"). But that's another issue.

bemoody avatar Oct 17 '25 01:10 bemoody

@bemoody , I agree that the old approach of formatting the base_date seemed fragile. I've updated this to use .strftime.

briangow avatar Oct 28 '25 18:10 briangow