Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` by briangow · Pull Request #510 · MIT-LCP/wfdb-python

briangow · 2024-10-16T22:00:12Z

This PR adds the capability for writing signals with unique samples per frame (samps_per_frame) to wfdb.io.wrsamp. This is typically the function that is used to write WFDB files. This was previously only possible to do by creating a Record first and using its wrsamp method to do the write.

@bemoody , I do feel like checking that the frames were uniform took a bit of extra code. I'm happy to leave it like this but also open to discussing other approaches.

I've added a couple of tests to check that this continues to work as expected.

tompollard · 2024-10-16T23:06:09Z

wfdb/io/record.py

-    if p_signal is not None and d_signal is not None:
+    signal_list = [p_signal, d_signal, e_p_signal, e_d_signal]
+    signals_set = sum(1 for var in signal_list if var is not None)
+    if signals_set != 1:


Is this intentionally changing behavior for calls where p_signal=None and d_signal=None? In the past, this would evaluate to False when e_p_signal=None and e_d_signal=None, but it now evaluates to True.

@tompollard , if all signals are passed as None a failure will occur further down the line. @bemoody , do we want to allow all signals to be None when calling wfdb.io.wrsamp? If we don't want to allow that then I will leave the code as I have it which requires that one of the signals is set.

If passing None for both signals (p_signal and d_signal) previously resulted in an error, then the new behavior is an improvement. Unless people were intentioanlly calling the function with both signals set to None previously, this change isn't a problem.

tompollard · 2024-10-17T02:05:11Z

wfdb/io/record.py

-    # required features.
+
+    # If samps_per_frame is a list, check that it aligns as expected with the channels in the signal
+    if len(samps_per_frame) > 1:


If samps_per_frame is not a list (e.g. it's an int), this will error out.

len(4)
TypeError: object of type 'int' has no len()

tompollard · 2024-10-17T02:21:16Z

wfdb/io/record.py

+            raise TypeError("Unsupported signal format. Must be ndarray or list of lists.")
+
+        # Check that the number of channels matches the number of samps_per_frame entries
+        if num_sig_channels != len(samps_per_frame):


as above, len(samps_per_frame) will error if samps_per_frame is an int.

wfdb/io/record.py

…_frame

briangow · 2024-10-18T18:29:50Z

@tompollard , if this failing test is of concern, could you guide me on what to do to resolve it:

would reformat /home/runner/work/wfdb-python/wfdb-python/wfdb/io/record.py
2 files would be reformatted, 36 files would be left unchanged.

tompollard · 2024-10-18T19:08:19Z

@tompollard , if this failing test is of concern, could you guide me on what to do to resolve it:

It looks like the style checks are failing (https://github.com/MIT-LCP/wfdb-python/actions/runs/11408864264/job/31747939807?pr=510). To fix, you'll need to make sure that the code conforms to black (install black, run black over the code to reformat).

bemoody · 2024-10-21T20:05:33Z

The following raises an error:

>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], fmt=["16"],
                e_p_signal=[numpy.array([0.0, 1.0, 2.0])],
                adc_gain=[1.0], baseline=[0])
TypeError: 'NoneType' object is not subscriptable

In this case, wrsamp should raise an exception to say that if e_p_signal is provided, then samps_per_frame must be too.

bemoody · 2024-10-21T20:05:50Z

The following raises an error:

>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], e_p_signal=[numpy.array([0.0, 1.0, 2.0])], fmt=["16"], samps_per_frame=[3])
TypeError: ufunc 'isinf' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

The problem here is that calc_adc_params doesn't handle the expanded case. calc_adc_params should use the same method to generate adc_gain and baseline as it does in the non-expanded case.

I think it should be straightforward to restructure calc_adc_params to take an expanded argument and to use e_p_signal instead of p_signal where appropriate.

briangow · 2024-10-28T19:20:08Z

The following raises an error:
>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], fmt=["16"],
                e_p_signal=[numpy.array([0.0, 1.0, 2.0])],
                adc_gain=[1.0], baseline=[0])
TypeError: 'NoneType' object is not subscriptable
In this case, wrsamp should raise an exception to say that if e_p_signal is provided, then samps_per_frame must be too.

@bemoody , thanks for catching this. I think this also applies for e_d_signal. I've added a check to make sure samps_per_frame is supplied when writing these expanded signals.

I've also updated the check to make sure samps_per_frame aligns with the channels in the signal, to consider the case when samps_per_frame is an int instead of a list.

Finally, I think we should allow passing samps_per_frame with a 1 for each channel to wrsamp when a mixed frequency signal is read in with smooth_frames=True:

rec = wfdb.io.rdrecord('/wfdb-python/sample-data/mixedsignals', smooth_frames=True)

print(rec.samps_per_frame)
[4, 4, 4, 2, 2, 1]

wfdb.io.wrsamp('written_mixedsignals', fs = rec.fs, units=rec.units, sig_name=rec.sig_name, base_date=rec.base_date, base_time=rec.base_time, comments=rec.comments, p_signal=rec.p_signal, samps_per_frame=[1,1,1,1,1,1], baseline=rec.baseline, adc_gain=rec.adc_gain, fmt=rec.fmt)

Please let me know if you disagree, or have other thoughts with respect to this case.

briangow · 2024-10-29T14:57:02Z

The following raises an error:
>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], e_p_signal=[numpy.array([0.0, 1.0, 2.0])], fmt=["16"], samps_per_frame=[3])
TypeError: ufunc 'isinf' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
The problem here is that calc_adc_params doesn't handle the expanded case. calc_adc_params should use the same method to generate adc_gain and baseline as it does in the non-expanded case.

I think it should be straightforward to restructure calc_adc_params to take an expanded argument and to use e_p_signal instead of p_signal where appropriate.

@bemoody , this is being addressed in a separate PR: #512 .

bemoody

Looks good

@ajadczaksunriselabs

This pull request adds a changelog for `v4.2.0`. The changelog is based on the following auto-generated summary of merge commits generated by GitHub: ``` ## What's Changed * bug-fix: Numpy ValueError when cheking empty list equality by @ajadczaksunriselabs in #459 * bug-fix: Pandas set indexing error by @ajadczaksunriselabs in #460 * fix for /issues/452 by @tecamenz in #465 * Use numpydoc to render documentation by @SnoopJ in #472 * build(deps): bump readthedocs-sphinx-search from 0.1.1 to 0.3.2 in /docs by @dependabot in #477 * Update style by @bemoody in #482 * Fix NaN handling in Record.adc, and other fixes by @bemoody in #481 * Set upper bound on Numpy version (numpy = ">=1.10.1,<2.0.0"). Ref #493. by @tompollard in #494 * Update actions to use actions/checkout@v3 and actions/setup-python@v4. by @tompollard in #495 * Fix: Indent code to ensure 'j' is within for-loop in GQRS algorithm by @tompollard in #499 * Add write_dir argument to csv_to_wfdb. Fixes #67. by @tompollard in #492 * Fix warnings by @cbrnr in #502 * README improvements by @bemoody in #503 * Change in type promotion. Fixes to annotation.py by @tompollard in #506 * Use uv by @cbrnr in #504 * Change in type promotion. Fixes to _signal.py by @tompollard in #507 * Test round-trip write/read of supported binary formats by @bemoody in #509 * Corrected typo and extended allowed types for MultiSegmentRecord by @agent3gatech in #514 * Allow expanded physical signal in `calc_adc_params` by @briangow in #512 * Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` by @briangow in #510 * Fix selection of channels when converting to EDF by @SamJelfs in #519 * Change in type promotion introduced in Numpy 2.0. Fixes to edf.py. by @tompollard in #527 * Bump dependencies for NumPy 2 compatibility by @cbrnr in #511 * Bump version to v4.2.0 and update notes on creating new releases by @tompollard in #497 ## New Contributors * @ajadczaksunriselabs made their first contribution in #459 * @tecamenz made their first contribution in #465 * @SnoopJ made their first contribution in #472 * @dependabot made their first contribution in #477 * @agent3gatech made their first contribution in #514 * @SamJelfs made their first contribution in #519 **Full Changelog**: v4.1.2...v4.2.0 ```

briangow requested a review from bemoody October 16, 2024 22:00

tompollard reviewed Oct 17, 2024

View reviewed changes

briangow added 4 commits October 18, 2024 14:09

update wfdb.io.wrsamp to allow writing a signal with unique samps_per…

1a9937f

…_frame

add tests for wfdb.io.wrsamp for a signal with unique samps_per_frame

d7b5e93

address failing tests and feedback

0e32209

correct signal name being set

def62e9

briangow force-pushed the bg_wrsamp_multiple_fs branch from 866fe10 to def62e9 Compare October 18, 2024 18:11

add missing import

11e8756

briangow added 2 commits October 19, 2024 10:01

reformatted with black package

f8084d3

reformatted with black package

1e93bee

add wfdb.io.wrsamp signal checks

7bc468b

reformatted with black package

6b0d317

bemoody mentioned this pull request Oct 31, 2024

Allow expanded physical signal in calc_adc_params #512

Merged

bemoody approved these changes Nov 25, 2024

View reviewed changes

bemoody merged commit 1d192c5 into main Nov 25, 2024

bemoody deleted the bg_wrsamp_multiple_fs branch November 25, 2024 20:24

bemoody mentioned this pull request Nov 25, 2024

Multi-frequency data with wfdb.wrsamp #336

Closed

Comments

Conversation

briangow commented Oct 16, 2024

Uh oh!

tompollard Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briangow Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

tompollard Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

tompollard Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

briangow Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

tompollard Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

briangow Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

briangow commented Oct 18, 2024

Uh oh!

tompollard commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bemoody commented Oct 21, 2024

Uh oh!

bemoody commented Oct 21, 2024

Uh oh!

briangow commented Oct 28, 2024

Uh oh!

briangow commented Oct 29, 2024

Uh oh!

bemoody left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tompollard Oct 16, 2024 •

edited

Loading

tompollard commented Oct 18, 2024 •

edited

Loading