Repository navigation
Add support for reading and writing .wfdb archive files - #541
tompollard wants to merge 16 commits into
Conversation
f98cedc to
31fcfdb
Compare
1901e3d to
d72055c
Compare
d72055c to
39e8b59
Compare
39e8b59 to
a80f917
Compare
|
I asked Claude Opus 5.5 for a review. Overall, the design looks good, but there are some issues that should be addressed (grouped into three categories). Points 1, 2, 3, and 5 must be fixed before merge. Points 4 and 6 should be fixed (they are very cheap but not blocking), and the remaining points 7, 8, 9, and 10 are optional (ideally for a follow-up PR). Bugs
Performance
Structure and cleanup
|
A .wfdb archive is a ZIP file holding the header, signal and annotation files of one record. rdrecord, rdsamp, rdheader and rdann now read from an archive when given its path, including the .wfdb extension. The header is the file named after the archive, or the only header in it. Signal and annotation files are read by the names that follow from the header, and a missing file is an error. rdrecord reads the header and signals from a single open archive. Multi-segment records and archives in cloud storage or on PhysioNet are not supported yet. An archive holds a single record: WFDBArchive refuses to write a second header file, so an archive can't be written that rdrecord cannot read. Annotation and other files can be added alongside the record.
wrsamp and Record.wrsamp take a wfdb_archive argument: a path creates a new archive containing the header and dat files of the record, and an open WFDBArchive lets several files be written to one archive. A new archive is written to a temporary file named after it, and moved into place only when writing succeeds, so a failed write leaves no partial archive and an existing archive at the same path is either fully replaced or left unchanged. The new archive gets the permissions of the archive it replaces, or the default permissions for new files.
a80f917 to
51ef012
Compare
ZIP can't represent dates before 1980, and ZipFile.write raises a ValueError for such files by default. This happens with files from reproducible builds (SOURCE_DATE_EPOCH=0), for example. These files now get the earliest date ZIP supports.
To give a new archive the default permissions for new files, the umask was read by setting it to 0 and back. The umask is process-wide, so files that another thread created in between came out world-writable. The temporary file is now created with os.open and mode 0o666, so the operating system applies the umask, instead of with tempfile.mkstemp, which uses 0o600. The permissions are only copied when an existing archive is replaced.
When an archive is discarded, the temporary file was only removed if closing the zip file succeeded, so a failure (such as a full disk) left it behind.
Using a WFDBArchive after closing it failed with an AttributeError on None. It now raises a ValueError saying that the archive is closed.
When wfdb_archive was a path, write_dir was silently ignored, so the archive ended up in the current directory. A relative archive path is now relative to write_dir, like the files of a record written without an archive. An absolute path is used as it is.
A path such as '100.WFDB' was treated as a plain record name, and WFDBArchive added another '.wfdb' to it. The extension is now compared without regard to case.
Signal files were copied before being added to an archive: tobytes() for dat files, and BytesIO.getvalue() for FLAC files. WFDBArchive.write now streams any bytes-like object into the archive, so the array (or the FLAC buffer) is passed without a copy. The archive entries are written as before, and ZIP64 is used for files of 2 GiB or more.
- read_header no longer checks the declared size of the header before reading it: the bounded read already enforces the limit, whatever size the archive declares. - The scan for header files is in one helper, used by header_file_name and _check_new_file. - exists() looks the file up with getinfo, instead of building the list of all files on every call.
|
Thanks for the thorough review! I've pushed a commit for each point I fixed. The tests added for 1–6 fail without their fix; 8 has a new test for the streamed writes, and 10 is a refactor covered by the existing tests. Bugs
Performance
Structure and cleanup
I'll rebase #583 onto these changes next. |
lukepayyapilli
left a comment
There was a problem hiding this comment.
Thanks Tom, checked 889c97df locally. The offline tests pass on Python 3.11 (132 tests), and ruff check and ruff format --check are clean. I also went back over @cbrnr's review. For 1 to 6, each new test fails with the source from before its fix and passes with it. For 8, test_archive_write_bytes_like checks the archive entries rather than the copy (it passes before the change too), and I measured the same as you: writing an 8 MB signal peaks at about 126 MB, with or without an archive, before and after the change. For 7, I see the same pattern: seeking 200 MB into a stored entry takes 0.044 s on 3.11 and no measurable time on 3.12, and a deflated entry takes about 0.07 s on both, so leaving 7 and 9 for the follow-up with #583 makes sense to me. A few things from reading archives made by other tools, and two questions about the format:
- Archives zipped on macOS: when Finder zips
100.heaand100.dat, it also adds__MACOSX/._100.heaand__MACOSX/._100.dat._header_names()counts the first as a second header, so if the archive is renamed (for examplerec2.wfdb),rdrecordfails withCould not find 'rec2.hea' ... (it contains several header files: ['100.hea', '__MACOSX/._100.hea']). The spec draft in wfdb/wfdb-spec#21 says the files are at the top level, so the suggestion only counts top-level headers, with a test that fails without it. It also gives a clearer error for a zipped folder (100/100.hea,100/100.dat):Could not find '100.hea'instead ofCould not find '100.dat'. An archive made with theziptool, where everything is deflated, and a renamed archive both read correctly. - Header compression: the outline in wfdb/wfdb-spec#21 says header files "should be stored uncompressed", and your last comment there says to start without any zip compression, but
_compress_type()deflates headers. Archives stay readable either way, but if headers were stored too, a reader such as the WFDB C library wouldn't need zlib at all. It's worth settling before archives are written in the wild; changing it would touch_compress_type(), the module docstring,test_create_archive_compressionand the docs. - Finding archives: section 4.1 of the outline has tools look for
100.wfdbwhen the loose files aren't found, whereas here an archive is only read when the path ends in.wfdb. Requiring the explicit path seems simpler and less surprising to me, but the spec draft should say whichever is intended.
Co-authored-by: Luke Payyapilli <[email protected]>
Co-authored-by: Luke Payyapilli <[email protected]>
|
@cbrnr Any objections to merging this? If you're okay with doing so, I'll [try to] work out how to roll in support for the .wfdb files in the C package. |
As discussed in wfdb/wfdb-spec#21 and #540 we would like to allow WFDB Records to be saved to a single file
.wfdb.This pull request introduces support for reading and writing
.wfdbarchive files: a ZIP file that holds the header, signal and annotation files of a single record.Implementation
The pull request has three commits:
rdrecord,rdsamp,rdheaderandrdannread from an archive when given its path, including the.wfdbextension (for examplewfdb.rdrecord("100.wfdb")).100.heain100.wfdb), or the only header in the archive. Signal and annotation files are read by the names that follow from the header, and a missing file is an error.rdrecordreads the header and signals from a single open archive. Archives are opened and closed within each call, so nothing is cached between calls.wrsampandRecord.wrsamptake awfdb_archiveargument: a path, or an openWFDBArchive.create_archive(file_paths, archive_path)puts the files of an existing record into an archive.Format details:
WFDBArchiverefuses to write a second header file, so it's always clear which record an archive holds. Annotation and other files can be added alongside the record.Not supported yet: multi-segment records, and archives in cloud storage or on PhysioNet. These raise a clear error. Multi-segment support would extend the one-record rule to "the main header plus the segment headers it lists", and is planned as a follow-up.
Example usage
Note:
Record.wrsampwrites the digital signal, so the record is read withphysical=Falseabove. Calling it on a record read with physical signals fails with an unclearTypeError. That's existing behaviour, not specific to archives, and could be improved separately.Tests
tests/test_archive.pycompares records read from archives with the same records read from separate files, including FLAC formats, multiple samples per frame and sample ranges. It also covers writing, annotations, the one-record rule, permissions, failed writes, and error cases such as missing files and unsupported multi-segment records.