MIT-LCP / MIT-LCP/wfdb-python

wrsamp incorrectly saves samps_per_frame when expanded=False

Open
#333 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Jupyter Notebook
Stars
853
Forks
322
PR merge metrics
No merged PRs in 30d

Description

When writing a record (header + signal file) using Record.wrsamp, the signal data may either be single-frequency (d_signal) or multi-frequency (e_d_signal).

If expanded=True, then wrsamp takes the data from e_d_signal (ignoring d_signal) and writes one or more multi-frequency signal files. In this case the record must also have a samps_per_frame attribute, and this attribute is written to the header file.

If expanded=False (default), then wrsamp takes the data from d_signal (ignoring e_d_signal) and writes one or more single-frequency signal files. In this case the samps_per_frame attribute is still written to the header file, which is completely incorrect.

Why would you have a Record that is single-frequency yet has samps_per_frame not equal to 1? Because that's what rdrecord does by default when you read a multi-frequency record.

So:

>>> import wfdb
>>> r = wfdb.rdrecord('43700001', pn_dir='mimicdb/437', sampto=10, physical=0)
>>> r.wrsamp()
>>> wfdb.rdrecord('43700001')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/benjamin/wfdb-python/wfdb/io/record.py", line 3542, in rdrecord
    return_res=return_res)
  File "/home/benjamin/wfdb-python/wfdb/io/_signal.py", line 1010, in _rd_segment
    sampfrom, sampto, smooth_frames)[:, r_w_channel[fn]]
  File "/home/benjamin/wfdb-python/wfdb/io/_signal.py", line 1151, in _rd_dat_signals
    sig_data = _blocks_to_samples(sig_data, total_process_samples, fmt)
  File "/home/benjamin/wfdb-python/wfdb/io/_signal.py", line 1460, in _blocks_to_samples
    sig[0::2] = sig_data[0::3] + 256 * np.bitwise_and(sig_data[1::3], 0x0f)
ValueError: could not broadcast input array from shape (30) into shape (75)

(Arguably it's wrong for rdrecord to set samps_per_frame when smooth_frames=True, but changing that behavior would be hard to reconcile with rdheader, and would prevent implementing automatic frame-smoothing as I suggested in https://github.com/MIT-LCP/wfdb-python/pull/313#issuecomment-952033980)

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with Record.wrsamp and compare how the expanded=True and expanded=False paths handle samps_per_frame. Reproduce the issue using rdrecord on the MIMIC record shown, then inspect the read path in wfdb/io/_signal.py. Done means a single-frequency record written with expanded=False can be read back without the incorrect frame setting, while expanded=True behavior remains valid.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
data
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.