Repository navigation
Reject aux notes that can't be written in wrann - #605
Conversation
tompollard
left a comment
tompollard
left a comment
There was a problem hiding this comment.
Thanks, this looks good! I checked that valid notes are written exactly as before (record 100's annotations and your Latin-1 example give byte-identical files to main), and that both invalid-note tests fail on main. Latin-1 also matches how rdann decodes the notes.
Unrelated to this PR, but I noticed it while testing: the wrann docstring says an aux note can be None for annotations without notes, but None is rejected with TypeError: Subelements of the aux_note field must be strings (on main too). Either the docstring or the check should change at some point.
|
@TheNetherWatcher please could you resolve the conflict? |
|
working |
The length of an aux note is stored in one byte and its characters as single bytes, but wrann didn't check either. A note longer than 255 characters wrapped the length and was read back as many extra annotations, and characters outside Latin-1 were silently replaced. Raise a ValueError for these notes before writing, and write the Latin-1 encoded bytes, which is what rdann decodes.
cc3e682 to
87046ce
Compare
|
Done, rebased onto |
Found by @tompollard while reviewing #605. The `wrann` docstring (and the `Annotation` example) say an aux note can be `None` for annotations without a note, but `None` was rejected: ```python wfdb.wrann("a", "atr", np.array([1, 5, 9]), symbol=["N", "N", "V"], aux_note=[None, "(VT", None]) # TypeError: Subelements of the aux_note field must be strings ``` `compact_fields` already turns empty notes into `None` before writing, so the writer handles `None`. Only `check_field` rejected it. It now skips `None` in `aux_note`, so `None` is written exactly like `""` (byte-identical files), and `rdann` reads both back as `""`. I kept the docstring as it is, since it already describes this. Tests: `None` and `""` notes give byte-identical files, and read back as `""`. The test fails on `main`. `pytest -m "not network"` passes (100 tests), and the docs build passes.
wrannwrites each aux note as a one-byte length followed by one byte per character, but it doesn't check that the note fits. The bytes are cast touint8, so values above 255 wrap silently:rdanndecodes each byte withchr, i.e. as Latin-1, so the notes that round-trip are those of at most 255 Latin-1 characters (the length byte also limits notes to 255 bytes in the WFDB format).Changes:
check_fieldnow raises aValueErrorfor an aux note that is longer than 255 characters or has characters outside Latin-1, before anything is written.field2byteswritesvalue.encode("latin-1")and takes the length from the encoded bytes. For valid notes the output is the same as before.wranndocstring mentions the limits, and there's a changelog entry.Tests: a round trip with a 255-character note and Latin-1 characters (
°,ü), and both kinds of invalid note raising the error without creating a file. The invalid-note tests fail onmain.pytest -m "not network"passes (99 tests), and the docs build passes.